Skip to content
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

Fix for #3210 without computing the Bayes network #3273

Merged
merged 7 commits into from
Dec 3, 2018

Conversation

lucianopaz
Copy link
Contributor

Fix for #3210 which uses a completely different approach than PR #3214. It uses a context manager inside draw_values that makes all the values drawn from TensorVariables or MultiObservedRV's available to nested calls of the original call to draw_values.

It is partly inspired by how Edward2 approaches the problem of forward sampling. Ed2 tensors fix a _value attribute after they first call sample and then only return that. They can do it because of their functional scheme, where the entire graph is recreated each time the generative function is called. Our object oriented paradigm cannot set a fixed _value attribute because we would want two independent calls to an RV's random, sample_prior_predictive or sample_posterior_predictive methods to give different results. This means that draw_values has to know the context on which it was used.

The original implementation of draw_values is almost entirely preserved. The lines that do the while missing_input loop, which could potentially slow down forward sampling, could be removed if we did a topological sort of params. Maybe we could consider doing that in a separate PR.

…istently with scipy.stats. Added test and updated example code.
…n PR pymc-devs#3214. It uses a context manager inside `draw_values` that makes all the values drawn from `TensorVariables` or `MultiObservedRV`s available to nested calls of the original call to `draw_values`. It is partly inspired by how Edward2 approaches the problem of forward sampling. Ed2 tensors fix a `_values` attribute after they first call `sample` and then only return that. They can do it because of their functional scheme, where the entire graph is recreated each time the generative function is called. Our object oriented paradigm cannot set a fixed _values, it has to know it is in the context of a single `draw_values` call. That is why I opted for context managers to store the drawn values.
return func(*values)
output = func(*values)
return output
print(param,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.

@junpenglao
Copy link
Member

I am in favor of merging this, any comment? @twiecki @aseyboldt

@lucianopaz
Copy link
Contributor Author

@junpenglao, I noticed that I still have to write to the release notes and do some minor changes in distributions that call draw_values more than once inside their random method, like the mixtures. I'll push them a bit later today.

…ultivariate distributions that make many calls to draw_values or other distributions random methods within their own random.
@twiecki
Copy link
Member

twiecki commented Dec 3, 2018

Great, this looks waaay better and simpler than before. I think we should merge this after merge conflicts are fixed.

@junpenglao junpenglao merged commit 686b81d into pymc-devs:master Dec 3, 2018
@junpenglao
Copy link
Member

Great work @lucianopaz ;-)

@twiecki
Copy link
Member

twiecki commented Dec 4, 2018

Indeed, if this doesn't demonstrate tenacity I don't know what would :). Great job!

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 this pull request may close these issues.

3 participants