Skip to content

Comments

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

Closed
ColCarroll wants to merge 1 commit intopymc-devs:masterfrom
ColCarroll:mixture-fix
Closed

[WIP] Allow size argument sampling from Normal distribution#2623
ColCarroll wants to merge 1 commit intopymc-devs:masterfrom
ColCarroll:mixture-fix

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