fix(interpolate): select instances by time, not list position#104
Merged
Conversation
Two compounding label bugs made the tutorial's headline claim unverifiable, though the interpolation itself was always correct. 1. Off-by-one: ml_instances_list[0] is the t=0 fit and [1] is t=1 (the list is built by 'for time in range(3)'), but both print blocks labelled them 'fit 1 (t = 1)' and 'fit 2 (t = 2)'. The reader was invited to bracket the interpolated t=1.5 value between two numbers that were actually the t=0 and t=1 fits. 2. The aggregator does not return fit order — verified against a real output directory it loads t=0, t=2, t=1. So the aggregator block printed a different 'fit 2' value than the in-memory block (49.96 vs 59.73) for the same three fits, presented without comment as if the round-trip had changed the result. Both blocks now select via instance_at_time() and print identical values, and the prose explains that aggregator order is not fit order. Interpolation was never affected: _value_map keys by the time attribute and sorts, so the t=1.5 result (54.834) was correct throughout. It now visibly brackets between 49.96 and 59.73, making the script's 'close to 55.0' claim actually demonstrable. Verified: exit 0. Notebook regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Found while clearing this script's stale NEEDS_FIX marker (PyAutoLabs/PyAutoFit#1411). Script always exited 0 and the interpolation was always correct — these are labelling bugs that made the tutorial's headline claim unverifiable.
1. Off-by-one in the labels (both blocks)
ml_instances_list[0]is the t=0 fit and[1]is t=1 — the list is built byfor time in range(3). Both blocks labelled themfit 1 (t = 1)andfit 2 (t = 2). The prose then invited the reader to bracket the interpolated t=1.5 centre between two numbers that were actually the t=0 and t=1 fits, so the check demonstrated nothing.2. The aggregator does not return fit order
Verified against a real
output/interpolate:That is why the aggregator block printed a different "fit 2" (59.73) than the in-memory block (49.96) for the same three fits — presented without comment, as if the round-trip changed the result.
Fix
Both blocks now select via a small
instance_at_time()helper, so position can never drift from time again, and the prose explains that aggregator ordering is not guaranteed to be fit order (a genuinely useful lesson here).Interpolation was never affected —
_value_mapkeys bytimeand sorts internally. After the fix both blocks print identical values and 54.834 visibly brackets between 49.96 and 59.73, making the script's "close to 55.0" claim demonstrable.Verified: exit 0, real run. Notebook regenerated.
🤖 Generated with Claude Code