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

[MAINT] Fix for #3346 #3352

Merged
merged 4 commits into from Jan 25, 2019

Conversation

Projects
None yet
2 participants
@lucianopaz
Copy link
Contributor

lucianopaz commented Jan 24, 2019

This fixes #3346. The draw_values function had a statement that said that if the name of a variable was in the supplied point, then the value for said variable would be taken from the point dictionary. This was too permissive, and different from what was done in _draw_value. There, the value could be taken from the point dictionary only if the variable had the attribute model, i.e. it was an RV instance instead of a Deterministic.

The downside of this PR is that we cannot force values to Deterministics in sample_posterior_predictive relying on the point parameter.

Fix for #3346. draw_values had a statement that said that if the name…
… of a variable was in the supplied point, then the value for said variable would be taken from the point dictionary. This was too permissive, and different from what was done in _draw_value. There, the value could be taken from the point dictionary only if the variable had the attribute model, i.e. it was an RV instance instead of a Deterministic.

@lucianopaz lucianopaz requested review from junpenglao , ColCarroll and twiecki Jan 24, 2019

@lucianopaz

This comment has been minimized.

Copy link
Contributor Author

lucianopaz commented Jan 24, 2019

@junpenglao, @ColCarroll, @twiecki, do you think it is ok that draw_values will ignore any Deterministics that are specified in the point dictionary?

@ColCarroll
Copy link
Member

ColCarroll left a comment

the test makes this look like a good change, but I have trouble figuring out what was wrong and how this code fixes it! i'll keep looking at it, but would appreciate some pointers!

Show resolved Hide resolved pymc3/distributions/distribution.py
Show resolved Hide resolved pymc3/distributions/distribution.py
@ColCarroll

This comment has been minimized.

Copy link
Member

ColCarroll commented Jan 24, 2019

Regarding your question, I think it should definitely ignore any Deterministics in the point argument. You could do really weird stuff otherwise.

Added comment explaining why we check for the model attribute of para…
…meters to get their values from the point dictionary.

@ColCarroll ColCarroll merged commit cdb69ec into pymc-devs:master Jan 25, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@ColCarroll

This comment has been minimized.

Copy link
Member

ColCarroll commented Jan 25, 2019

Thanks for these very technical fixes, @lucianopaz!

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