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

[MAINT] Fix for #3346 #3352

Merged
merged 4 commits into from
Jan 25, 2019
Merged

[MAINT] Fix for #3346 #3352

merged 4 commits into from
Jan 25, 2019

Conversation

lucianopaz
Copy link
Contributor

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.

… 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
Copy link
Contributor Author

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

Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

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!

pymc3/distributions/distribution.py Show resolved Hide resolved
pymc3/distributions/distribution.py Show resolved Hide resolved
@ColCarroll
Copy link
Member

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

…meters to get their values from the point dictionary.
@ColCarroll ColCarroll merged commit cdb69ec into pymc-devs:master Jan 25, 2019
@ColCarroll
Copy link
Member

Thanks for these very technical fixes, @lucianopaz!

@lucianopaz lucianopaz deleted the iss3346 branch February 24, 2019 07:53
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.

Prediction on New Data Fails with Deterministic Variable
2 participants