Skip to content

Fix redefinitions #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Fix redefinitions #598

merged 1 commit into from
Jan 15, 2024

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Jan 15, 2024

Description

This is a refactor designed to make these parts of code slightly more inspectable by type checkers like mypy.

  1. We use @overload to distinguish between the two distinct usages of general_toposort for which different sets of arguments are specified.
  2. When a variable represents a callable, we differentiate between the variable definition and the function definition. The principle is as follows:

Worse:

if cond:
    def chosen_function()
        ...
else:
    def chosen_function()
        ...

Better:

def function_option_1():
    ...
def function_option_2():
    ...

if cond:
    chosen_function = function_option_1
else:
    chosen_function = function_option_2

The first option is worse because the definition of the underlying function changes based on the runtime logic. The second option is cleaner and more explicit because the functions are statically defined, and the choice of one or the other is made explicit.

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@maresb maresb mentioned this pull request Jan 15, 2024
11 tasks
@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (ccc08ba) 80.92% compared to head (c2456c3) 80.91%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
- Coverage   80.92%   80.91%   -0.01%     
==========================================
  Files         162      162              
  Lines       46627    46634       +7     
  Branches    11399    11401       +2     
==========================================
+ Hits        37732    37735       +3     
- Misses       6667     6670       +3     
- Partials     2228     2229       +1     
Files Coverage Δ
pytensor/graph/basic.py 88.76% <81.81%> (-0.33%) ⬇️
pytensor/graph/rewriting/basic.py 72.99% <33.33%> (-0.05%) ⬇️

@ricardoV94 ricardoV94 merged commit f737996 into pymc-devs:main Jan 15, 2024
@maresb maresb deleted the fix-redefinitions branch January 15, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants