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

WIP Refactor Distribution and random variables #2833

wants to merge 1 commit into
base: master


None yet
5 participants
Copy link

aseyboldt commented Feb 2, 2018

Work in progress!!

Refactoring the Distribution class and the various random variable classes a bit. The goal here is to decouple those, to make things cleaner, and I think it should also make it easier to play with different backends.
It should also make it much easier to implement support for changing the dimension of free variables after model creation (if they depend on a shared variable for instance), and to get rid of the test_value thing in theano.

This changes a bit how we think about the shapes of variables. Previously the shape was set statically by inspecting the test_value of the created theano variable, and then fixed in the distribution. With this PR, each distribution has two shapes: atom_shape and param_shape. The atom shape is the shape of one observation, so for most distributions this will just be (). For eg the MVNormal this will be (n,). param_shape is the shape implied by the parameters of the distribution. So if we have a pm.Normal('a', mu=np.zeros(5), sd=1), the param shape is (5,). Since parameters can be theano variables the param_shape is not known statically, but can only be computed given values for all previous variables. (The same should probably be true for the atom_shape, but I haven't implemented that yet).
The distribution itself doesn't know the shape of the random variable anymore, this really belongs into the random variable.
Since we do not use the test_values to infer that shape anymore, we need to keep track of the variable shapes in the model. It keeps dicts of shapes and default values (model._RV_shapes, model._default_values), and uses those shapes and default values to infer the shape of newly created variables `.

@junpenglao junpenglao added the WIP label Feb 2, 2018


This comment has been minimized.

Copy link

fonnesbeck commented Feb 2, 2018

This is great, and long overdue. I did not like atom_shape at first glance, but the more I think about it, the more I like it. Its hard to be clear about the distinction, and this does a good job of it.


This comment has been minimized.

Copy link

junpenglao commented Feb 2, 2018

During the lab meeting @aseyboldt also said maybe we should change it to be inline with what is used in tensorflow distribution, which i agree.


This comment has been minimized.

Copy link

ahmadsalim commented Jul 20, 2018

I have taken a look at the code from the discussion in #3101, and I think this PR looks very exciting. Is there any status on what remains to be done for this PR?



This comment has been minimized.

Copy link

twiecki commented Dec 22, 2018

Closing because of age. Feel free to reopen if you want to fix this up.

@twiecki twiecki closed this Dec 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment