Repository navigation
fix: use per-iteration lambda in EM iteration history records - #3346
Open
David Bustos Usta (dfbustosus) wants to merge 1 commit into
Open
David Bustos Usta (dfbustosus) wants to merge 1 commit into
David Bustos Usta (dfbustosus) wants to merge 1 commit into
Conversation
_iteration_history_records stamped every iteration's records with self.core_model_settings.probability_two_random_records_match — the final converged value — instead of the probability from that iteration's entry in _core_model_settings_history. As a result the match weights and m/u history chart tooltips reported the converged lambda for all iterations except the last, while the lambda iteration chart (fed by _lambda_history_records) was correct. Closes moj-analytical-services#1619
This branch has not been deployed
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 #1619.
Problem
_iteration_history_recordsinsplink/internals/em_training_session.pystamped every iteration's records withself.core_model_settings.probability_two_random_records_match— the final converged value — instead of the probability stored in that iteration's entry of_core_model_settings_history:Effect: the match-weights and m/u interactive history chart tooltips reported the converged λ for every iteration, while
probability_two_random_records_match_iteration_chart(fed by_lambda_history_records, which already reads the per-iteration value) was correct — exactly the inconsistency reported in #1619 / the linked discussion.Fix
One line: read
probability_two_random_records_matchfrom the per-iterationcore_model_settingsin the loop, as suggested by Robin Linacre (@RobinL) in the discussion.Tests
New
tests/test_em_iteration_history_records.py:[0.001, 0.002, 0.004]must produce records carrying each iteration's own probability (fails on the old code — all records got0.004)The session is built with
EMTrainingSession.__new__plus a stubbed settings history, since the property only reads_core_model_settings_history— no database needed.Verification
pytest tests/test_em_iteration_history_records.py tests/test_expectation_maximisation.py→ 6 passed, 3 deselected (the real EM-session tests still pass with the change)ruff checkclean on both files