Skip to content

Reconsider not allowing shape/ellipsis in dist classmethods #5701

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

Closed
ricardoV94 opened this issue Apr 8, 2022 · 2 comments · Fixed by #5719
Closed

Reconsider not allowing shape/ellipsis in dist classmethods #5701

ricardoV94 opened this issue Apr 8, 2022 · 2 comments · Fixed by #5719

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 8, 2022

When working in #5690, I realized we have a bug when doing the following:

import pymc as pm
x = pm.GaussianRandomWalk.dist(init=pm.Normal.dist(), steps=5, shape=(3, ...))
print(x.shape.eval())  # [3, 3, 6]; should be [3, 6]
print(x.eval())  # ValueError

This happens because we have to resize init to be of the same size of x, as the GaussianRandomWalk (GRW) is a composite distribution of init + innovations. We do this in dist, and I simply did not consider that shape could have an ellipsis.

In general, this sort of issue will be prevalent in most "composite" distributions, such as LKJCorr, Censored, Mixture, and so on, where we need to resize some of the the dist arguments to match size, shape, dims or else ask users to to it themselves. We can make things quite simpler for us if we don't allow the ellipsis or shape argument into the dist classmethod, as we will only have to worry about size in that case.

@ricardoV94 ricardoV94 changed the title Reconsider allowing shape/ellipsis in dist classmethods Reconsider not allowing shape/ellipsis in dist classmethods Apr 8, 2022
@ricardoV94
Copy link
Member Author

Actually, should be fine if we just act as if shape does not exist if we need to interact with size...

@ricardoV94
Copy link
Member Author

Actually that's not good, because super().dist() does not recall dist (it cannot). Maybe such resizing should be done in the rv_op

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant