Fix MAG L1C gap-filling to follow spec section 7.3.4 step 3; pass T013-T016, T024#2
Draft
Fix MAG L1C gap-filling to follow spec section 7.3.4 step 3; pass T013-T016, T024#2
Conversation
Make L1C generate missing timestamps at each gap's declared vector rate instead of a hardcoded 0.5 s cadence, and filter out one-sample micro-gaps near Config-mode transitions so those boundary artifacts do not trigger burst interpolation. Also fix interpolation window handling by masking on the epoch column, including the burst slice endpoint, and allowing the filtered interpolator to use the final valid edge sample. Update the L1C unit expectations and unskip the T015/T016 validation cases.
Fixes 5 root-cause bugs in MAG L1C gap-fill processing that caused validation tests T015 and T016 to fail. Rewrites the gap-fill architecture to use a plan-first approach aligned with the MAG Science Algorithm Document (IMAP-MAG-SW-009-01B, section 7.3.4): - Fix 2D array indexing in fill_normal_data (mask on first axis only) - Use rate-based gap cadence instead of hardcoded 0.5s - Fix off-by-one in burst data slicing - Replace micro-gap filter with _find_rate_segments for Config transitions - Add GapFillPlan dataclass separating "what to fill" from "how to fill" - Implement spec step 3: tC selection, BM decimation, NM-aligned shifting - Add 4 new unit tests and row count assertions in validation tests All 106 MAG tests pass including all 10 validation tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the full NM-grid merge in build_gap_fill_plan with BM-only synthetic epochs clamped to actual burst coverage. Replace the fallback gap-fill with pre-computed rate segments and output-cadence trim. Switch interpolate_gaps source tracking to O(1) dict lookup. Tighten validation to exact row counts, 1ms timestamp tolerance, and ancillary column checks. 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)
Addresses IMAP-Science-Operations-Center#1993 — MAG L1C validation tests T015 and T016 were failing.
Root cause: The existing L1C gap-fill code did not implement the algorithm specified in section 7.3.4 step 3 of IMAP-MAG-SW-009-01B (MAG Baseline Science Algorithm Doc, issue 5 rev 1). The spec requires: find the nearest BM timestamp (tC) to the gap start (tA), decimate BM samples around tC, shift the decimated timestamps by (tA − tC), and trim to the open interval (tA, tB). The old code instead built a fixed-cadence scaffold timeline and tried to fill it from burst data after the fact — producing wrong timestamps, wrong row counts, and wrong interpolation results for any test case with non-2Hz rates or config-mode transitions.
All 114 MAG tests now pass, including all 10 L1C validation tests (T013–T016, T024 × mago/magi).
Why this PR changes so much code
The short answer: the old gap-fill approach was structurally incompatible with the spec, not just parametrically wrong. Fixing it required replacing the control flow, not just tweaking values.
The old code used a scaffold-first approach:
This fails because:
The new code uses a plan-first approach that mirrors the spec directly:
GapFillPlan(frozen dataclass) with the exact synthetic epochs and their BM source indices — BEFORE any interpolationThis separation of "what timestamps to produce" from "how to interpolate them" is what makes the new code correct — and it's why so many lines changed. The actual algorithm in each new function is straightforward; the bulk of the diff is the new function structure and docstrings.
What's preserved unchanged:
mag_l1c(),select_datasets(),generate_empty_norm_array(),vectors_per_second_from_string(),remove_missing_data(), and the overallprocess_mag_l1c()flow. The existing L1A, L1B, L1D, and L2 tests are completely unaffected.Bugs fixed
1. Gap-fill algorithm did not follow spec section 7.3.4 step 3
The spec (pages 29–32) prescribes: find tC (nearest BM sample to tA), decimate around tC at
burst_rate / norm_ratestep, shift decimated timestamps to NM alignment viatA + period_offsets × nm_spacing, trim to (tA, tB). The old code skipped all of this — it generated a regular grid at fixed cadence and searched for the nearest burst sample per grid point. This produced correct-looking results only when the NM rate happened to be 2 Hz and BM coverage was complete.2. Nanosecond float round-trip precision loss
Timestamp values (int64 nanoseconds, ~10^18) were being passed through float64 arithmetic, which has only ~15.9 significant digits. This caused silent ±1–100 ns drift in epoch values. Fixed by adding
_to_int64_ns()/_to_int64_ns_scalar()helpers that keep all timestamp math in int64 and only usenp.rint()when forced to convert from float.3. Hardcoded 0.5s gap cadence ignored NM data rate
generate_missing_timestamps()always used 0.5s spacing regardless of the actual NM rate. The spec (section 4.4.1, page 9) says cadence depends on the NM data rate before the gap. T016 proves this: MAGo uses rate 4 → 0.25s spacing, MAGi uses rate 1 → 1.0s spacing. The old hardcoded 0.5s only appeared correct because T013–T015 all use N2_2 mode (rate 2 = 0.5s). Fixed: spacing is nowint(1e9 / rate).4. Spurious micro-gaps at Config mode transitions
When MAG switches science modes (e.g., N2_2 → B64_8), the rate change creates timestamp spacings that
find_gaps()misidentified as data gaps. The old code had a fragile post-hoc filter deleting gaps under 1.5s. Fixed:_find_rate_segments()walks backward from configured transition points to find where observed cadence matches the new rate, thenfind_all_gaps()applies each segment's rate when checking for gaps.5. CIC filter edge loss from missing
extrapolate=Truelinear_filtered()calledremove_invalid_output_timestamps()which trimmed output timestamps to the input range — but the CIC filter's delay truncation had already shortened the input, causing valid output timestamps to be discarded. The spec's reference code (page 32) uses IUS (InterpolatedUnivariateSpline) which extrapolates by default. Fixed: passextrapolate=Truetolinear().6. Wrong 2D array indexing in
fill_normal_data()Used 1D boolean mask on 2D
vectorsarray, causing shape mismatch. Fixed: apply mask to first axis only (vectors[mask, :]).7. Off-by-one in burst data slicing
interpolate_gaps()usedburst_gap_endexclusively, dropping the last sample needed for interpolation. Fixed: include the boundary sample.New functions (in
mag_l1c.py)_to_int64_ns()/_to_int64_ns_scalar()GapFillPlan(dataclass)_find_rate_segments()get_vecsec_dict()find_nearest_epoch_index()build_decimated_indices()build_synthetic_epochs()_find_segment_for_time()build_gap_fill_plan()build_gap_fill_plans()build_timeline_from_gap_plans()_build_fallback_gap_fill_plan()build_default_gap_fill_plans()Modified functions
process_mag_l1c()build_gap_fill_plans()→build_timeline_from_gap_plans()→interpolate_gaps()fill_normal_data()interpolate_gaps()GapFillPlanobjects; source tracking via dict lookup + set (wasnp.searchsorted); no-NM fallback delegates tobuild_default_gap_fill_plans()find_all_gaps()_find_rate_segments()instead of single global rategenerate_missing_timestamps()generate_timeline()New tests
test_find_nearest_epoch_index_prefers_earlier_on_tie— deterministic tie-breakingtest_find_segment_for_time_uses_precomputed_segments— segment resolutiontest_build_decimated_indices_includes_anchor— anchor always presenttest_generate_missing_timestamps_uses_gap_rate— rate-aware cadencetest_build_gap_fill_plan_uses_shifted_burst_timestamps— spec step 3 shifttest_build_gap_fill_plan_regularizes_burst_jitter— zero-jitter output from jittery inputtest_build_gap_fill_plan_uses_observed_burst_transition_boundary— real T015-scale timestampstest_build_gap_fill_plans_match_step_three_shifted_timeline— end-to-end timeline checktest_find_all_gaps_uses_observed_transition_boundary— rate transition detectiontest_generate_timeline_rate_gap— rate-aware gap fill in legacy pathtest_find_rate_segments— backward-walk segment detectiontest_build_gap_fill_plan— core plan correctnessValidation test changes
assert len(expected_output.index) == len(l1c["epoch"].data)) — was ±1 tolerancerange,compression,compression_width, andinterpcolumns when present in expected outputSpec references
Recommended pre-merge validation
Per @alastairtree: run on early MAG commissioning data (where modes changed several times) and send the MAG team a produced L1C CDF for review.