Skip to content
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

Dimshuffle does not need input broadcastable info #979

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Aug 22, 2024

Description

Make DimShuffle less concerned about it's input static broadcastable type when dropping dims, since no behavior depends on it being specified at runtime.

But there is no reason the input broadcastable type had to be known in advance, since no behavior of the Op changes with that information. It's true that dropping it only works when the runtime shape is of length 1, but that will naturally fail when we try to do it for different shape lengths.

Alternatively / in addition we could plaster specify_broadcastable during make_node. Beyond this potential extension, I see no reason why DimShuffle should be so strict.

This fixes the bug reported in #969

Related Issue

@ricardoV94 ricardoV94 force-pushed the dimshuffle_does_not_need_input_broadcastable_info branch from 0a667f6 to 278faf6 Compare August 22, 2024 12:44
@ricardoV94 ricardoV94 force-pushed the dimshuffle_does_not_need_input_broadcastable_info branch 5 times, most recently from 0556779 to 57ff2b7 Compare August 24, 2024 10:07
Copy link

codecov bot commented Aug 24, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.73%. Comparing base (1c2bc8f) to head (2425c9a).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/tensor/elemwise.py 82.75% 2 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #979   +/-   ##
=======================================
  Coverage   81.73%   81.73%           
=======================================
  Files         183      183           
  Lines       47734    47721   -13     
  Branches    11611    11601   -10     
=======================================
- Hits        39016    39007    -9     
+ Misses       6523     6521    -2     
+ Partials     2195     2193    -2     
Files with missing lines Coverage Δ
pytensor/sparse/sandbox/sp.py 74.61% <100.00%> (-0.20%) ⬇️
pytensor/tensor/basic.py 91.54% <100.00%> (ø)
pytensor/tensor/extra_ops.py 87.73% <100.00%> (-0.04%) ⬇️
pytensor/tensor/fft.py 82.29% <100.00%> (ø)
pytensor/tensor/inplace.py 100.00% <100.00%> (ø)
pytensor/tensor/math.py 91.27% <100.00%> (-0.01%) ⬇️
pytensor/tensor/random/rewriting/jax.py 97.08% <ø> (ø)
pytensor/tensor/rewriting/basic.py 94.15% <100.00%> (ø)
pytensor/tensor/rewriting/elemwise.py 91.14% <100.00%> (+0.29%) ⬆️
pytensor/tensor/rewriting/jax.py 82.81% <ø> (ø)
... and 4 more

@ricardoV94 ricardoV94 marked this pull request as ready for review August 24, 2024 11:57
@ricardoV94 ricardoV94 force-pushed the dimshuffle_does_not_need_input_broadcastable_info branch from d573dd3 to bf9f33b Compare August 24, 2024 18:48
@ricardoV94 ricardoV94 force-pushed the dimshuffle_does_not_need_input_broadcastable_info branch from bf9f33b to 47f2dc9 Compare October 7, 2024 07:27
@ricardoV94 ricardoV94 force-pushed the dimshuffle_does_not_need_input_broadcastable_info branch from 47f2dc9 to 4b834aa Compare October 7, 2024 13:11
@ricardoV94 ricardoV94 force-pushed the dimshuffle_does_not_need_input_broadcastable_info branch from 4b834aa to 2425c9a Compare October 7, 2024 13:18
@ricardoV94 ricardoV94 merged commit e88117e into pymc-devs:main Oct 8, 2024
60 of 61 checks passed
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.

Bug in gradient of fft.rfft DimShuffle should be happy to drop dims if they have length 1 at runtime
2 participants