-
-
Notifications
You must be signed in to change notification settings - Fork 377
Fix bug - stepsize from windowed adaptation is reset to 1 when term_buffer
== 0
#3345
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
Conversation
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
I think this is incorrect. The condition added here means that the stepsize adaptation is never restarted, even as the metric changes, which means the dual averaging is over the entire warmup phase, not just the most recent window. I think the correct solution will need to modify |
I think the correct condition is to update the current At the end of metric adaptation, |
no, I think it's simpler than that - there's an off by one error in the logic of stan/src/stan/mcmc/windowed_adaptation.hpp Lines 86 to 100 in 6d0d863
|
I've added a bunch of tests to check that this logic doesn't affect the overall number of warmup iterations and that the stepsize is not reset to 1 at the final iteration. |
For the tests, I think it would be better to check that the third column isn't ==1 for the first sampling iteration, rather than relying on the string outputs. I think there's also still an issue here, possibly a new one? I'm testing 3 configurations which should all have identical metric adaptation (at least, if my intuition here is correct), but the
(this is for the bernoulli model with seed 1234). I think the issue is that the |
@WardBrian : Happy Birthday!!! |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
agreed - there is definitely a problem here. working on it. |
…v/stan into issue/3023-term-buffer-0
discretion is the better part of valor. refactoring not worth it. will submit fix for bug and unit tests in a new PR. |
Submission Checklist
./runTests.py src/test/unit
make cpplint
Summary
The underlying bug occurs when
stepsize_adaptation.restart
is called immediately beforestepsize_adaptation.complete_adaptation
; the former resets valuex_bar_
to 0; the latter returnsexp(x_bar_)
which is 1. Added a check tocomplete_adaptation
which doesn't update the stepsize whenx_bar_
is 0.I found this bug after refactoring the behavior of the
windowed_adaptation
object in order to make sure that the adaptation schedule was being computed correctly. The code uses namesphase1
,phase2
, andphase3
to track the adaptation schedule. Thewindowed_adaptation
object exposes a counter,cur_iter_
which is incremented by the adapter objects,adapt_diag_e_nuts
et al.Unit tests on the
windowed_adaptation
check that the schedule is computed properly.Unit tests on the
hmc_nuts_diag_e_adapt_sampler
check that the adapted stepsize matches the stepsize used during sampling.Intended Effect
If sampler config sets 'term_buffer=0', preserve the evolved stepsize from previous adaptation phases.
How to Verify
Unit tests
Side Effects
N/A
Documentation
N/A
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: