Document mir_history offset; fix waypoint assertion - #245
Open
neuromechanist wants to merge 1 commit into
Open
Conversation
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.
Closes #161, and repairs a test my own #241 broke.
#161 was mostly already done
Both of its testable claims already have tests in
test_ng_convergence.py—test_mir_history_survives_keep_best_restoreandtest_mir_history_empty_after_save_load. Only the third, informational item was outstanding.Verified before documenting it:
llcomes from the E-step accumulator at the top of the loop,_update_parametersruns, and the MIR waypoint is computed after. Somir_history_[i]reflects the parameters after iterationi's update whilell_history[i]is the likelihood of the parameters before it. Zipping them by index compares different parameter states. Now stated on both the backend attribute and theAMICA.mir_history_docstring.A regression from #241
Running the whole file rather than a subset surfaced
test_mir_history_survives_keep_best_restorefailing ondev:I bisected it: the test passes at
257d20e(the commit before #241 merged) and fails atc80fb33(the merge). #241 caused it. That PR's gm-ordering change shifts multi-model trajectories by ~1e-6, which moved themin_dllstop to iteration 55; MIR waypoints land on multiples ofmir_step=5, so the last one is at 50, not 54.The assertion
last_it == len(ll_history) - 1was fragile from the start — it only held while the stop happened to coincide with a waypoint, and any trajectory perturbation could break it. It now asserts what the test is actually about: the last waypoint is the highest multiple ofmir_stepat or below the final iteration, and is within one interval of the end, so a restore that truncated the trajectory would still fail it.Why CI did not catch it
The test is not
slow-marked, so CI runs it, and CI was green on #241's branch. I have not established why the branch and the squashed merge behave differently here; the practical lesson is that I ran a subset locally on #241 and would not have seen this either way. Flagging rather than leaving it implicit.