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

Defer initEpsilon() to start of next iteration #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

perrydv
Copy link
Contributor

@perrydv perrydv commented Jun 18, 2024

This PR defers the call to initEpsilon() and associated resetting of a few warmup-related variables from the end of one iteration to the beginning of the next. This is intended for two reasons, both having to do with the case of other samplers operating on the model. (Hence, it should not be relevant if HMC is the only sampler.) These changes are only for the NUTS_sampler, not the "classic" version. The two reasons are:

  1. I'm not 100% sure but it looks like initEpsilon() could leave the model out of state, specifically with logProb values that are not up to date. I see values(model, nodes) <- is used to reset node values, but superficially it looks like logProb values would also need to be put back in state. If that is a non-issue, I might be forgetting why. Leavining logProbs out of state will be no problem if the NUTS sampler is about to run (as in this PR), because it will update all nodes and end by leaving the model in state. But if the NUTS sampler is finishing and another sampler will run, it could cause an incorrect update. This would only happen around 5ish times spread throughout a warmup schedule, so it might not cause catastrophic results.
  2. If other samplers have operated, then initEpsilon() will operate after them and just before the NUTS update. It is just a guess that this might be slightly better than having initEpsilon() operate after NUTS and before other samplers, making its determination of an initial epsilon value possibly contingent on old values of other parameters.

I am running tests locally and don't remember if we have CI set up for this package. About to find out.

@danielturek
Copy link
Member

@perrydv Confirming that the old behaviour (without this PR) of the initEpsilon method can leave the model out-of-state, where stored log probs are inconsistent with current values in the model.

Testing for nimbleHMC package is automatic. Your creating this PR did trigger testing, which passed. So, for better or for worse, this change did not trigger testing to fail. That could be interpreted as a good thing (the consequences of this out-of-state model are so rare / minor ...), or as a bad thing (the automated package testing is so rudimentary ...)

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.

2 participants