-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
Implement lifting for Subtensor
s and DimShuffle
s applied to multivariate RandomVariable
s
#1040
Comments
If I understand your proposal, at least for the Dimshuffle case, it doesn't seem that the ability to lift the operator justifies adding an extra parameter to all multivariate distributions. It should however be safe to lift it through non-support dimensions, although I am a bit fuzzy about how you would do that for parameters with different number of core dimensions. It would be nice to lift a subtensor operation but that can definitely only be done if it does not remove partial entries from the support dimensions. |
The costs of adding such a parameter, especially at the
See the docstrings and comments in those rewrites; both should provide high and low-level descriptions of what they're doing and why. |
Just to be clear, here I meant developer costs, not performance costs. |
I guess my confusion is why do we care about lifting DimShuffles in particular, and not other operators like Obviously it's easier to manipulate shapes. But being easier doesn't make it more relevant. My guess is that the "issue" comes from Dimshuffles being introduced very frequently by Elemwise operators? But those cases are relatively simple to eliminate by just appending Alternatively we could just stop adding Dimshuffles for Elemwise operations as is discussed in another issue. |
This is not an "either ... or ..." situation, and lifting exponentials through
If you think this specific type of rewrite, or
The "issue" is very real and relevant, because No, appending
That would fix nothing; it would only somewhat reduce the occurence of |
I think it would still work, you only need to convert them to a "MeasurableDimShuffle". Our rewrites aren't looking for pure RVs anymore, and being a MeasurableCumsum or MeasurableDimShuffle should pose exactly the same limitations. I fail to see what is special about Dimshuffles that we want to get rid of them (other than the fact we can). Outside of Aeppl, it seems to me that your proposal above transfers the responsibility of Dimshuffling to RandomVariables perform method, by adding an auxiliary parameter that determines the axis of support. That's the part I don't see the point of. Again, because I don't see what is the problem that Dimshuffles introduce.
It seems to me like we can always do it. Whether we want or not is a different question. The same I am raising for the more general lift. Conceptually it is similar to what you are proposing except it relies on already supported behavior of RVs and not on a new feature.
In general, we can always easily canonicalize, implicit batched dimensions to size, but we don't have to.
We can skip the intermediate step and go directly to the last one in a single rewrite. If you have multiple parameters it's equally easy, but you need to broadcast the (batched) shapes of the parameters first. We have that machinery already written in Aesara as well. Anyway this is a tangential proposal to expand the scope of the DS lift rewrite that already exists, to special cases that don't interfere with core dimensions and are applicable to both univariate and multivariate distributions alike. It's not an or question.
I don't follow what you mean here. |
This is not the AePPL repo, and a solution in AePPL is not a solution here or anywhere else that Also, I'm well aware of how AePPL works now, because I made it work that way; that's also why I'm able to say that the Before going any further with this topic, please tell me how your alternative(s) address the basic rewriting challenges introduced by If you don't know what those rewriting challenges are, then start your questions there, because I've mentioned that they're an important reason for doing this work, but you keep avoiding this point.
After this point, the rewriting you describe is essentially what the existing If the answer(s) are specific to the Otherwise, let's keep these discussions on the topic of this issue instead: i.e. adding multivariate support to |
You touch upon a very relevant point here, and I'll follow up on it later, but the basic idea is that we might be able to absorb that |
Our current rewrites,
aesara.tensor.random.opt.local_subtensor_rv_lift
andlocal_dimshuffle_rv_lift
do not support multivariateRandomVariable
s.This is in part due to a fundamental restriction involving the way inputs are mapped to support dimensions (e.g.
DirichletRV
), but not exclusively.For at least the
DimShuffle
case, it's possible that an additional parameter indicating the support dimension(s) could be used to work around this restriction.From aesara-devs/aeppl#150 (comment):
The text was updated successfully, but these errors were encountered: