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

Projects
None yet
3 participants
@lucianopaz
Copy link
Contributor

lucianopaz commented Nov 27, 2018

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.

lucianopaz added some commits Nov 14, 2018

Fix for #3225. Made Triangular `c` attribute be handled consistently …
…with scipy.stats. Added test and updated example code.
Rebase to branch 'master' of https://github.com/pymc-devs/pymc3 so th…
…at py27 joblib does not make tests fail
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 `_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.

@junpenglao junpenglao requested a review from aseyboldt Nov 27, 2018

return func(*values)
output = func(*values)
return output
print(param,

This comment has been minimized.

@junpenglao

junpenglao Nov 27, 2018

Member

Don't forget to remove this

This comment has been minimized.

@lucianopaz

lucianopaz Nov 27, 2018

Contributor

Oops.

@junpenglao

This comment has been minimized.

Copy link
Member

junpenglao commented Nov 29, 2018

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

@lucianopaz

This comment has been minimized.

Copy link
Contributor

lucianopaz commented Nov 29, 2018

@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.

Added release notes and draw values context managers to mixture and m…
…ultivariate distributions that make many calls to draw_values or other distributions random methods within their own random.
@twiecki

This comment has been minimized.

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.7%) to 88.718%
Details
@junpenglao

This comment has been minimized.

Copy link
Member

junpenglao commented Dec 3, 2018

Great work @lucianopaz ;-)

@twiecki

This comment has been minimized.

Copy link
Member

twiecki commented Dec 4, 2018

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

@lucianopaz lucianopaz deleted the lucianopaz:draw_values_context_man branch Dec 4, 2018

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