Repository navigation
Conversation
When init_buffer + window + term_buffer exceeds num_warmup, set_window_params reduced the stages to 15%/75%/10% but returned without restart(), so adapt_next_window_ kept the constructor's value (0 + 0 - 1, i.e. UINT_MAX). The adaptation window never ended and the diag_e / dense_e metric was never updated, although the warning says the reduced windows are used. Call restart() in that branch as in the other.
Author
|
So apparently an upstream test relies on the problematic behavior this is trying to address, so I think I'll just leave this here for someone else to consider. I'm now kinda confused what the actually expected behavior is here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This offers a fix to an inconsistency between documented and actual behavior I noticed. Whether this is the best way to resolve the inconsistency is perhaps up to discussion because in some sense this is a tradeoff (see "Intended effect"). If you prefer another approach let me know.
AI use disclosure: This was developed with the help of Claude Code (Opus 5.5).
Submission Checklist
./runTests.py src/test/unitmake cpplintSummary
When
init_buffer + window + term_buffer > num_warmup(with the defaults:num_warmupfrom 20 to 149),windowed_adaptation::set_window_paramsreduces the stages to 15%/75%/10% but returns without callingrestart().adapt_next_window_keeps the constructor's0 + 0 - 1(UINT_MAX), so the adaptation window never ends and thediag_e/dense_emetric is never updated. The fix callsrestart()in that branch, as the other branch does.Intended Effect
Warmup follows the documented schedule when the stages do not fit. The reference manual says they "are scaled down", and the warning lists the reduced windows; with the fix the sampler uses them. A run with the default windows now equals a run with the announced windows given explicitly, and the metric is adapted from
num_warmup = 20, the documented lower limit, instead of only once the stages fit (150 with the defaults).In the measurement under Side Effects this raises the minimum bulk ESS per gradient for
num_warmup30–149 by 3× (eight schools) to several hundred× (normals with sd e⁻³…e³). If maintainers prefer develop's actual behaviour (the initial metric, with the step size tuned over the whole warmup), the other way to make code and warning agree is to keep the code and change the warning to say that the metric is not adapted.How to Verify
McmcVarAdaptation.learn_variance_reduced_windowsrunsnum_warmup=100with the default windows and expects the variance estimated from the 75 draws of the reduced window. It fails on develop, where the variance stays at its initial value. No existing expectation changes.Side Effects
Draws change for adaptive
diag_e/dense_eruns whose windows are reduced. Measured on CmdStan 2.39.0 (same sampler code as develop), the branch emulated by passing the reduced windows explicitly (same window state as the fix), 20 seeds on 5 configurations: normals with sd e⁻³…e³; logistic regression; correlated normal (diag and dense); eight schools. develop → this branch:num_warmupAt 20–50 the step size is retuned over only 2–5 iterations after the metric update, as with such windows given explicitly on develop. Pooled eight-schools estimates match a long reference run on both sides, but at
num_warmup=30the spread of per-run E[tau] grows from 0.15 to 0.41. Ametric_filemetric is now re-estimated in these runs;init_buffer=0 term_buffer=0 window=0keeps it. I can share the scripts.Documentation
None.
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):
Jáchym Barvínek jachymb@gmail.com
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: