Skip to content

[WIP] Allow size argument sampling from Normal distribution #2623

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
wants to merge 1 commit into from

Conversation

ColCarroll
Copy link
Member

This came up in #2614, and is a super partial fix that I wanted opinions on before implementing everywhere. Consider the following two code samples:

x = pm.Normal.dist(mu=np.array([1, 2]), sd=np.array([.2, .3]))
x.random(size=5)

This returns a (5,2) array with this change, as (arguably!) desired. On master, it throws an exception (which is the bug in #2614)

with pm.Model():
    pm.Normal('X', mu=np.array([1, 2]), sd=np.array([.2, .3]), shape=9)
    trace = pm.sample()

This returns a (500, 2) array with this change, ignoring the shape=9 argument, which I think is bad. On master, it throws an exception, which I think is good. My understanding is that shape is intended to be a hint as to the shape of mu/sd? It shouldn't be hard to throw an exception with this change (just change the line to self.shape = self.shape or np.broadcast(...).shape).

I can add this change pretty easily to all the distributions, but wanted input before proceeding!

@junpenglao
Copy link
Member

The shape handling is a bit of a mess underneath IMO, you are very brave to tackle it ;-)
To me, the difficulties are the distributions that takes vector as parameters (e.g., pm.Categorical, pm.Multinomial) - the shape there could easily be messed up (e.g., p.shape=(5,) or (5,1) some times makes a difference).

Also, the following works on master:
For the first case:

x = pm.Normal.dist(mu=np.array([1, 2]), sd=np.array([.2, .3]), shape=(2,))
x.random(size=5)

For the second case:

with pm.Model():
    pm.Normal('X', mu=np.array([1, 2]), sd=np.array([.2, .3]), shape=(9,2))
    trace = pm.sample()

Which to me is that in the current setup, user needs to be more discipline and provide the right shape when it is not observed.

@twiecki
Copy link
Member

twiecki commented Oct 9, 2017

Shapes, rearing their ugly head again. After long discussions on this, I think the shape kwarg is not that bad. Users just need to know that if the shape can't be inferred, you must pass the shape of the final RV you want.

@ColCarroll Your fix for random() looks like the behavior we want and I think raising an exception in your other example is right too.

@junpenglao
Copy link
Member

Yeah not sure if it is still feasible, but I think having both kwarg of shape (kind of the raw shape of RV given the parameters), and size (repeatition of the RV, which broadcast the raw shape of the RV to this size) would be ideal to make things more explicit.

@AustinRochford
Copy link
Member

@ColCarroll i think this is a reasonable fix for a tricky problem, thanks for prototyping it!

@ColCarroll
Copy link
Member Author

I was so naive then. Closed by #2983

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.

4 participants