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 mcmc api transforms None for potential_fn #2266

Merged
merged 4 commits into from
Jan 21, 2020

Conversation

ludkinm
Copy link
Contributor

@ludkinm ludkinm commented Jan 17, 2020

Fixes #2272.

This PR fixes a logic gap for assign self.transforms in the MCMC api.
It can be the case that both MCMC.transforms is None and MCMC.kernel.transforms is None
in which case MCMC.transforms should be set to an empty dict.
This bug appears when using HMC/NUTS with a potential_fn.

Note sure on the style guide for inline comments so will leave them as is.

pyro/infer/mcmc/api.py Outdated Show resolved Hide resolved
pyro/infer/mcmc/api.py Outdated Show resolved Hide resolved
Copy link
Member

@neerajprad neerajprad left a comment

Choose a reason for hiding this comment

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

LGTM, once the spacing issue is fixed. Thanks for fixing this!

@neerajprad
Copy link
Member

I just pushed a fix to release a patch for 1.2 with this fix.

@neerajprad neerajprad added the WIP label Jan 21, 2020
@neerajprad
Copy link
Member

There are a few issues still with multiprocessing, I'll either fix it here or put out a separate PR.

@neerajprad
Copy link
Member

ludkinm - Could you review the minor fixes that I made for #2272, and let me know if that's in line with your use case?

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @ludkinm and @neerajprad, especially for your comments about what is going on!

@neerajprad could you dismiss your previous review?

@neerajprad
Copy link
Member

@fehiepsi - can we merge this? I would like to fix #2272 and release a patch update.

@fehiepsi
Copy link
Member

@neerajprad your review is still in change request status, which does not allow me to merge.

@fehiepsi fehiepsi dismissed neerajprad’s stale review January 21, 2020 21:57

Dismiss the review

@fehiepsi fehiepsi merged commit b1e6020 into pyro-ppl:dev Jan 21, 2020
@fehiepsi
Copy link
Member

Sorry, I just know that I can dismiss your review :)

@ludkinm
Copy link
Contributor Author

ludkinm commented Jan 22, 2020

Sorry I have been away from the office so didn't respond.
@neerajprad thanks for your fix, it works in my use case too - my python is rusty and forgot that if statements are evaluated lazily.

@ludkinm ludkinm deleted the ludkinm-transforms-fix branch January 22, 2020 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] MCMC run throws exception when using num_chains>1 as of Pyro 1.2.0.
4 participants