Minimally Fix MAG L1C gap-fill bugs: pass validation tests T015 and T016#2885
Closed
Minimally Fix MAG L1C gap-fill bugs: pass validation tests T015 and T016#2885
Conversation
Five surgical bug fixes to the existing L1C gap-fill code, with no architectural changes, to un-skip and pass validation tests T015/T016 while keeping T013, T014, and T024 passing. Fixes: #1993 Co-Authored-By: Claude Opus 4.6 <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
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.
Summary (From Claude)
Five surgical bug fixes to the existing MAG L1C gap-fill code to un-skip and pass validation tests T015 and T016, while keeping T013, T014, and T024 passing. No architectural changes, no new functions or classes, no rewrite of the pipeline flow.
This supersedes the earlier, heavier draft PR at sapols#2 (
claude/mag-l1c-fix), which rewrote the gap-fill architecture with new dataclasses, helper functions, and a BM-only algorithm. After feedback from @alastairtree and @maxinelasp -- that the existing code was developed collaboratively with the MAG team and that deviations from the algorithm document may be intentional simplifications -- we went back todevand identified the smallest set of changes needed to make the validation tests pass. The result is ~25 lines changed across 4 files instead of hundreds.Closes #1993
Bug fixes
Fix A: 2D array indexing in
interpolate_gaps()(mag_l1c.py)filled_norm_timelineis an (n, 8) array, but the gap-timeline extraction was comparing the entire 2D array against scalar timestamps without specifying a column. The boolean mask broadcasted across all 8 columns, producing garbled results.Fix: Index column 0 explicitly:
filled_norm_timeline[:, 0][...]Fix B: Off-by-one burst slice in
interpolate_gaps()(mag_l1c.py)burst_endwas clamped tolen(burst_epochs) - 1, but the subsequent Python slice[burst_start:burst_end]excludes the right endpoint, silently dropping the last burst sample from interpolation.Fix: Use
burst_start : burst_end + 1in slices.Fix C: CIC filter edge loss in
linear_filtered()(interpolation_methods.py)After the CIC filter truncates samples to compensate for group delay, the filtered timestamps are shorter than the originals.
linear()then callsremove_invalid_output_timestamps(), which clips valid output timestamps to the now-shorter filtered range -- discarding data at the edges that should be extrapolated.Fix: Pass
extrapolate=Truetolinear(), matching the spec reference CIC code which uses IUS (extrapolates by default).Fix D: Spurious micro-gaps at config mode transitions (
mag_l1c.py)When MAG switches vector rates, the declared rate-transition timestamp does not perfectly align with the actual cadence change in the data.
find_all_gaps()splits the timeline at declared transitions, andfind_gaps()misidentifies the boundary misalignment as 1-sample gaps. These phantom gaps cause downstream interpolation failures.Fix: Filter out gaps whose duration spans at most 2 cadence periods (i.e., a single missing sample artifact) after
find_all_gaps()returns.Fix E: Hardcoded 0.5s cadence in
generate_missing_timestamps()(mag_l1c.py)Gap-fill timestamps were always generated at 0.5s (2 Hz) intervals regardless of the gap declared vector rate. For rate=4 (0.25s) or rate=1 (1.0s) gaps, this produced the wrong number of timestamps and misaligned with the MAG team expected validation outputs.
Fix: Read the rate from the gap descriptor third element (
gap[2]) and compute cadence asint(1e9 / vectors_per_second). Theint()cast also fixes nanosecond floating-point precision issues innp.arange().Bonus: Dead code removal
Removed a no-op
np.delete(filled_norm_timeline, timeline_index)call whose return value was never assigned.Files changed
mag/l1c/mag_l1c.pymag/l1c/interpolation_methods.pytests/mag/test_mag_validation.pypytest.skip()guard for T015/T016tests/mag/test_mag_l1c.pyTest results
All 102 MAG tests pass, including the previously-skipped T015 and T016 for both mago and magi:
Why this approach instead of the earlier PR
The earlier PR (sapols#2) took a comprehensive approach: rewriting the gap-fill pipeline with
GapFillPlandataclasses,build_gap_fill_plan(),_find_rate_segments(), and a BM-only interpolation algorithm. While technically sound, it replaced code that was developed in close collaboration with the MAG instrument team and made assumptions about which deviations from the algorithm document were bugs vs. intentional simplifications.This PR takes the opposite approach: start from the existing code, identify the specific bugs preventing T015/T016 from passing, and fix only those. The 5 bugs found are all unambiguous correctness issues (wrong array dimensions, off-by-one slicing, clipped interpolation range, phantom gap detection, wrong cadence). No judgment calls about algorithm design were needed.