Skip to content

Allow rebuilding graphs when output type depends on input values #280

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 2 commits into from
Jun 23, 2023

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Apr 20, 2023

Closes #254
Closes #253

@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch 3 times, most recently from c956086 to 86c6ac0 Compare April 21, 2023 08:39
@ricardoV94 ricardoV94 requested a review from ferrine April 24, 2023 08:53
@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch from 86c6ac0 to 2c429c0 Compare April 24, 2023 08:55
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2023

Codecov Report

Merging #280 (e0febc4) into main (8112576) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #280   +/-   ##
=======================================
  Coverage   80.39%   80.40%           
=======================================
  Files         156      156           
  Lines       45397    45400    +3     
  Branches    11106    11104    -2     
=======================================
+ Hits        36497    36502    +5     
+ Misses       6694     6691    -3     
- Partials     2206     2207    +1     
Impacted Files Coverage Δ
pytensor/graph/basic.py 89.21% <100.00%> (+0.01%) ⬆️
pytensor/graph/op.py 87.19% <100.00%> (+0.12%) ⬆️
pytensor/tensor/basic.py 90.77% <100.00%> (+0.01%) ⬆️
pytensor/tensor/extra_ops.py 89.00% <100.00%> (+0.01%) ⬆️
pytensor/tensor/random/op.py 97.48% <100.00%> (+0.01%) ⬆️
pytensor/tensor/shape.py 91.70% <100.00%> (-0.04%) ⬇️
pytensor/tensor/var.py 87.57% <100.00%> (+0.14%) ⬆️

... and 2 files with indirect coverage changes

@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch from 2c429c0 to 54b4b94 Compare May 25, 2023 09:19
@ricardoV94 ricardoV94 changed the title Do not return Constants for shape Allow rebuilding graphs when output static shape depends on input values May 25, 2023
@ricardoV94 ricardoV94 changed the title Allow rebuilding graphs when output static shape depends on input values Allow rebuilding graphs when output type depends on input values May 25, 2023
@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch from 54b4b94 to 2a5b419 Compare May 25, 2023 09:24
@ricardoV94
Copy link
Member Author

I test that the Shape Op is present even when static shapes are present.
I also added a second commit to fix #253

@ricardoV94 ricardoV94 requested a review from ferrine May 25, 2023 09:24
@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch 2 times, most recently from 306f531 to d15f8ca Compare May 25, 2023 09:51
@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch from d15f8ca to 214a447 Compare May 25, 2023 09:58
@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch from 214a447 to 24c20dc Compare May 25, 2023 10:43
@ricardoV94 ricardoV94 requested a review from ferrine June 21, 2023 08:48
@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch from 24c20dc to e0febc4 Compare June 21, 2023 08:48
for i, (curr, new) in enumerate(zip(self.inputs, new_inputs)):
if curr.type != new.type:
if (curr.type != new.type) or output_type_depends_on_input_value:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super clear the branching logic from the outsider glance, can you please add comments that explain what exactly happens in each branch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

These nodes must always be rebuilt in non-strict mode
@ricardoV94 ricardoV94 force-pushed the keep_shape_dependencies branch from e0febc4 to 0522b2a Compare June 23, 2023 09:14
@ricardoV94 ricardoV94 merged commit 2665737 into pymc-devs:main Jun 23, 2023
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.

Static type shapes break graph dependency clone_replace doesn't try to rebuild nodes when input types don't change
3 participants