-
Notifications
You must be signed in to change notification settings - Fork 112
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
Alloc vs BroadcastTo vs Second #367
Comments
Actually this may touch on a more general question of when to allow I guess this depends on other inplace Operations. For instance, if you have a |
There's also pytensor/pytensor/scalar/basic.py Lines 826 to 830 in e20dd0b
pytensor/pytensor/scalar/basic.py Lines 3850 to 3864 in e20dd0b
It seems that there is a rough organization in the rewrites, where pytensor/pytensor/tensor/rewriting/basic.py Lines 408 to 416 in e20dd0b
pytensor/pytensor/tensor/rewriting/math.py Lines 2045 to 2046 in e20dd0b
Would be useful to understand why these we defined as the "canonical" forms. Maybe easier to merge multiple equivalent broadcasts than if they were represented as Alloc? I am pretty sure we don't need 3 separate Ops to do the same thing here :) |
Otherwise, I imagine we would need that:
We could simply remove it and continue having Alloc always be a fully allocated output. More discussion in #361 (comment) |
Description
Alloc
provides the same functionality asBroadcastTo
, and seems to be the default introduced in PyTensor rewrites in graphs this:This is introduced usually by this helper:
pytensor/pytensor/tensor/rewriting/basic.py
Lines 90 to 117 in 36161e8
It doesn't make sense to have two operators for the same functionality, so we should decide which one to support.
This was added in Aesara in aesara-devs/aesara#145
The original issue mentions the alloc / vs view question: aesara-devs/aesara#36, but it seems that could easily be achieved by a single Op by manipulating the
view
flag.The text was updated successfully, but these errors were encountered: