-
Notifications
You must be signed in to change notification settings - Fork 121
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
Rewrite reshapes that only expand or squeeze dims #1200
Conversation
b792532
to
5f3d19a
Compare
5f3d19a
to
6f9c9ca
Compare
3e23871
to
60340c7
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (97.67%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1200 +/- ##
==========================================
- Coverage 82.28% 82.27% -0.02%
==========================================
Files 186 186
Lines 47987 48018 +31
Branches 8629 8634 +5
==========================================
+ Hits 39486 39505 +19
- Misses 6347 6362 +15
+ Partials 2154 2151 -3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some questions/nitpicks -- I think it could be merged as-is but I want answers!
60340c7
to
516f5b9
Compare
@jessegrabowski I addressed your comments, let me know if it makes you happy :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code always makes me happy, I just hope my reviews always make you happy too : )
We canonicalize so that reshape is only used for the behavior that is unique to it (mixing dimensions). Expand_dims and squeeze-like behavior are canonicalized into the respective forms of DimShuffle. This allows other rewrites that know how to reason about these (but not reshape, which is too flexible), such as in the following graph:
Which before this PR generated this graph:
And now simplifies:
Or a naive broadcast + elemwise comparison:
Which used to generate this graph:
And now generates:
Which is great because it avoids materializing the full broadcasted inputs!
This example is not absurd, it showed up in this PyMC model: https://gist.github.com/ricardoV94/f986686ce86511b293c5dd6be374e51d
Also fixed a bug in local_useless_reshape, that may be behind some failures that @tanish1729 detected
Closes #845
Closes #1123
Related to #883
📚 Documentation preview 📚: https://pytensor--1200.org.readthedocs.build/en/1200/