MAG L1C: continue previous day's timeline across the day boundary by sapols · Pull Request #3323 · IMAP-Science-Operations-Center/imap_processing

sapols · 2026-07-07T20:21:41Z

Closes #2925.

Summary

When a MAG L1C processing day opens with a gap, continue the previous day's normal-mode timeline across the day boundary instead of starting a fresh grid. This was developed and reviewed as draft PR sapols#10 (the full review discussion lives there): following @maxinelasp's review of an earlier revision and her new day-boundary unit tests (#3318), the draft was reworked into the much simpler implementation here - one optional previous-day input instead of a neighbor-file list, no L1C neighbors, no future days, and no synthetic validation fixtures.

What it does

mag_l1c() and process_mag_l1c() accept an optional previous_day_dataset (the previous day's norm L1B for the same sensor). When the processing day opens with a gap - including days with no normal-mode data at all - the timestamps generated for that gap continue the previous day's grid: anchored on its last sample within its own 24-hour day (trailing buffer samples past midnight are excluded), stepping at its ending cadence (snapped against the known MAG rates with _is_expected_rate). Only the clock is inherited; neighbor vectors never enter the output. Without a usable previous-day file, single-day processing still succeeds unchanged; the one behavioral difference that applies even on such days is the timestamp-precision fix described under Tests (gap-fill timestamps at 8+ vectors per second can move by up to 128 ns relative to previously generated products).

On the CLI side, the Mag L1C branch splits dependencies by start date using ProcessingInputCollection.get_valid_inputs_for_start_date: current-day L1B files are the one-or-two inputs exactly as before, and a previous-day norm L1B (delivered by a date_range entry in imap_mag_dependencies.yaml, see companion) routes to previous_day_dataset. Mode/sensor validation lives in mag_l1c, which ignores unusable neighbors with a warning.

Review feedback incorporated

All inline comments from @maxinelasp's review on the draft PR are addressed: no keyword-only marker and both optional datasets trail the signature, the no-neighbor behavior is documented, the redundant branch in mag_l1c() is gone with logging moved into process_mag_l1c(), _day_window_ns is renamed _expected_day_ns and the duplicate test helper _ttj2000_day_bounds is deleted in favor of it, cadence detection loops the known rates through _is_expected_rate, the anchor is the last timestamp inside the previous 24-hour day, inheritance triggers on any gap at the beginning of the day (not only no-NM days), and the CLI helper is gone in favor of get_valid_inputs_for_start_date with validation inside mag_l1c. Per the review summary, only the previous day is used and only L1B is accepted.

Tests

The two tests from #3318 are cherry-picked here with authorship preserved and their xfail markers removed; both pass. (#3318 is still open - if it merges first, this branch will be rebased to drop the duplicate commit.) Two details those tests surfaced, flagged for @maxinelasp:

generate_missing_timestamps routed integer gap bounds through np.rint (float64), which shifts generated timestamps by up to 64 ns at 2025-era TTJ2000 magnitudes. Fixed here; without it the zero-jitter assertions cannot hold. Note this fix is shared machinery: for normal-mode rates of 8+ vectors per second (whose periods are not multiples of 128 ns), gap-fill timestamps can now differ from previously generated products by up to 128 ns and occasionally by one row at a gap edge - the new values are the exact intended grid. Rates of 1, 2, and 4 vectors per second are byte-identical to before.
Per @maxinelasp's guidance that nanosecond precision is not expected at the instrument level, the new grid-continuity assertions compare timestamps at the ~1 us precision the MAG validation tests already use (rtol=0, atol=1e3 ns), and the fixture keeps its original 73 ms phase offset. (Background: the pipeline stores epochs in float64 columns whose spacing at these magnitudes is 128 ns, so exact-equality assertions were unsatisfiable for arbitrary phase offsets.)

New unit tests cover: unusable neighbors ignored (burst mode, wrong sensor, L1C source, unknown cadence, missing epochs), whole-day inheritance when no NM data exists, buffer samples past midnight excluded from the anchor, and CLI routing with and without a previous-day dependency.

Scope deferred to a follow-up issue

Tracked in #3324: T018 (inheriting from the previous day's L1C, which needs an L1C-on-L1C dependency design from the infra team), the T019 vs T024 spec conflict, MAG-approved validation data for T017-T019, gap management beyond gaps at the beginning of the day, and int64 timestamp carriage.

Companion (infrastructure)

IMAP-Science-Operations-Center/sds-data-manager#1471 adds date_range: ["-1d", "0d"] to the MAG L1C norm L1B inputs in imap_mag_dependencies.yaml so the previous day's file is actually delivered in production. Until that merges and deploys, the inheritance path stays dormant and processing is unchanged.

Verification

Full non-external test suite passes; pre-commit run --all-files is clean.

Remaining Tasks

Review/finalize/update the follow-up issue (MAG L1C day-boundary follow-ups: T018/T019, MAG validation data, and gap-management scope #3324) before merging, in case feedback on this PR changes the follow-up scope.

…AP-Science-Operations-Center#2925) When the processing day opens with a gap, generate the missing normal-mode timestamps on the previous day's grid - inheriting its ending cadence and phase from an optional previous-day norm L1B dataset - so the L1C timeline stays regular across the day boundary. With no (usable) previous-day file, behavior is unchanged and single-day processing still succeeds. - mag_l1c()/process_mag_l1c() gain an optional previous_day_dataset. The inherited grid anchors on the last sample within the previous 24-hour day and snaps its cadence against the known MAG rates via _is_expected_rate. With no normal-mode data at all, the whole window continues that grid. - Mag CLI splits L1C dependencies by start date with get_valid_inputs_for_start_date; the previous day's norm L1B (delivered via a date_range entry in imap_mag_dependencies.yaml) routes to previous_day_dataset. Mode/sensor validation lives in mag_l1c. - generate_missing_timestamps no longer routes integer nanosecond gap bounds through float64 (np.rint), which shifted generated timestamps by up to 64 ns at TTJ2000 scales. - Un-xfail the day-boundary tests from IMAP-Science-Operations-Center#3318; their phase offset moves to a multiple of 128 ns because the float64 epoch columns cannot represent other offsets exactly at 2025-era epochs. Shared test helper _ttj2000_day_bounds is replaced by the production _expected_day_ns.

- Ignore (with a warning) a previous day dataset that has no epoch variable instead of raising KeyError. - Log skipped inheritance (too few pre-midnight samples, unknown cadence) at warning level to match the other unusable-neighbor paths. - Select the previous-day file by the job descriptor (norm + sensor) rather than "norm" alone, so a wrong-sensor file can never shadow the right one. - Drop the "exactly as before" claim from the mag_l1c docstring: the generate_missing_timestamps precision fix means gap fills at 8+ vec/s can differ from previously generated products by up to 128 ns even without a previous-day file.

sapols · 2026-07-07T20:24:20Z

@maxinelasp Thanks for the review in the draft PR and for suggesting this simplified approach. I reworked the PR to do everything you suggested (and updated the description accordingly): only one optional previous-day dataset (no list, no future days), and only norm L1B is accepted; an L1C (or burst, or wrong-sensor) neighbor is ignored with a warning so we still process with one day of data. I'll open a follow-up issue to track the L1C-inheritance variant (T018), the T019 vs T024 spec conflict, and everything else needing MAG input.

Additionally, your tests caught a real precision bug, and surfaced a storage limitation. Flagging both since it required a small change to your test code:

Real bug your tests caught: generate_missing_timestamps sent integer gap bounds through np.rint, which returns float64. At 2025-era TTJ2000 magnitudes, that shifted every generated timestamp by up to 64 ns. Fixed; your zero-jitter assertions can't hold without it. One product-side heads-up that comes with the fix: regenerated L1C files can show gap-fill timestamps shifted by up to 128 ns at 8+ vec/s rates compared to previously generated products (that's the corrected exact grid, not a regression, so don't be alarmed if a diff against older files flags it).
One constant changed in _build_cross_day_l1b(): I changed phase_ns from 73_000_000 to 73_000_064. The pipeline stores epochs in float64 columns, and at these magnitudes float64 can only represent multiples of 128 ns. With a 73 ms offset every grid point is a rounding tie, so stored timestamps jitter +/-64 ns and the exact-equality assertions are unsatisfiable for any implementation. A multiple-of-128 offset keeps the test's intent fully. If you'd rather keep 73 ms and use tolerance-based asserts (atol=128 ns), I'm happy to do that instead. Longer-term, if MAG cares about exact nanoseconds, carrying the timeline as int64 through the pipeline could go in the follow-up issue (I'll mention it in the issue description).

sapols · 2026-07-07T20:24:33Z

        difference_ns = int(1e9 / int(gap[2]))
+    gap_start = gap[0]
+    gap_end = gap[1]
+    if not np.issubdtype(np.asarray(gap).dtype, np.integer):


This was the precision problem in generate_missing_timestamps the new tests uncovered

This would be much stronger if the method actually validated it's inputs and raised an exception if gap was the wrong type, and then fix the place where gap is generated to be in the right type.

sapols · 2026-07-07T20:24:42Z

+    # nanoseconds is 128 ns. A non-multiple offset makes every grid point a rounding
+    # tie, so the stored timestamps jitter +/- 64 ns and exact-equality assertions
+    # cannot hold for any implementation.
+    phase_ns = 73_000_064


This is the one constant value I changed

maxineofficial · 2026-07-07T20:26:44Z

Per previous discussions with MAG, we aren't expecting the timestamps to have nanosecond precision because it's not possible at the instrument level. Broadly speaking, the "close enough" atol values in the other tests are the expected precision.

Restore the original 73 ms phase offset in the day-boundary fixture and compare grid-continuity timestamps with rtol=0, atol=1e3 ns - the ~1 us precision the MAG validation tests already use - instead of exact equality, per review guidance that nanosecond precision is not expected at the instrument level. (Float64 epoch columns resolve only multiples of 128 ns at 2025-era TTJ2000 magnitudes, so exact equality would also constrain fixture phases artificially.)

sapols · 2026-07-07T20:56:54Z

Per previous discussions with MAG, we aren't expecting the timestamps to have nanosecond precision because it's not possible at the instrument level. Broadly speaking, the "close enough" atol values in the other tests are the expected precision.

That makes sense, thanks @maxinelasp. Based on that, I switched the new grid-continuity assertions to the same ~1 µs precision the validation tests use (rtol=0, atol=1e3 ns) and restored your original 73 ms phase offset (33faa5d). Updated the PR description accordingly. I kept the np.rint fix since the code should still generate the grid exactly internally (and the gap bookkeeping relies on exact integers), the tests just no longer demand nanosecond agreement in the stored output.

sapols · 2026-07-09T17:18:12Z

@alastairtree @tmplummer @laspsandoval Can I ask at least one of you to review this PR please? Maxine told me she will not have time to before she leaves IMAP.

alastairtree

Looks pretty good - thank you!

I think "grid" is the wrong terminology - I think you mean timeline and it is called that in other places in the codebase? Find some of the naming of stuff a bit abstract/clearly AI generated and hard to follow. Better to stick to the physics of the instrument - it measures the field as a vector with a timestamp. Depending on the mode the vectors+timestamps appear at different rates. Happy to have a chat about the instrument any time if that context stuff is useful, or read our instrument manual.

Have added some minor nitpicks. I don't know what your general code house style guide but mine is probably a bit more concise, except for variables and methods which should be as concrete and descriptive as necessary to make the code readable without comments as much as possible.

Let us know once data has started appearing with this fix in place. Thanks!

alastairtree · 2026-07-10T11:09:13Z

            # Input datasets can be in any order, and are validated within mag_l1c
            if len(input_data) == 1:


Suggested change

# Input datasets can be in any order, and are validated within mag_l1c

if len(input_data) == 1:

# input_data is the burst mode L1B file, normal mode L1B file, or both, and appears in any order

if len(input_data) == 1:

alastairtree · 2026-07-10T11:19:50Z

+
+def _previous_day_grid(
+    previous_day_dataset: xr.Dataset, midnight_ns: int, day_start_ns: int
+) -> tuple[int, int] | None:


I find this method to be confusingly named and very verbosely commented to the point of confusion. Suggest you get your AI code writing companion to dial down the verbosity a bit.

I think this method is just getting the last vector timestamp from the previous day and the rate of vector generation?

So call it that?

def _get_last_timestamp_and_rate_from_previous_day_in_ns( previous_day_dataset: xr.Dataset, midnight_ns: int, day_start_ns: int ) -> tuple[int, int] | None:

alastairtree · 2026-07-10T11:28:43Z

+    steps_to_window = max(1, -((anchor_ns - day_start_ns) // period_ns))
+    grid_start_ns = anchor_ns + steps_to_window * period_ns
+
+    return grid_start_ns - period_ns, rate


Wouldn't it just be simpler to return anchor_ns or as i think of it - last_timestamp_before_midnight

alastairtree · 2026-07-10T11:30:03Z

+        ):
+            # A gap at the beginning of the day: continue the previous day's grid up
+            # to the first real sample instead of the current day's own grid.
+            inherited_gap_start_ns, inherited_rate = inherited_grid


Helpful comment but since the log message says exactly the same thing in a more readable way why not just delete the comment? Similarly in the elif

alastairtree · 2026-07-10T11:34:56Z

+        # The inherited gap starts one period before the first in-window grid point,
+        # because interpolate_gaps only fills points strictly inside a gap. That extra
+        # leading point must not appear in the output timeline.
+        new_timeline = new_timeline[new_timeline != inherited_gap_start_ns]


Shouldn't this be > inherited_gap_start_ns not != inherited_gap_start_ns or have I misunderstood?

I think that new_timeline > inherited_gap_start_ns and new_timeline != inherited_gap_start_ns will return the same mask in this case but I agree that using greater than is clearer for someone who doesn't know the specific logic that generated the values in new_timeline. In fact, if I am understanding correctly, new_timeline = new_timeline[1:] would also produce the same result?

alastairtree · 2026-07-10T11:37:54Z

        difference_ns = int(1e9 / int(gap[2]))
+    gap_start = gap[0]
+    gap_end = gap[1]
+    if not np.issubdtype(np.asarray(gap).dtype, np.integer):


This would be much stronger if the method actually validated it's inputs and raised an exception if gap was the wrong type, and then fix the place where gap is generated to be in the right type.

alastairtree · 2026-07-10T11:45:48Z

+    return norm_day1, norm_day2, burst_day2, meta
+
+
+def test_process_mag_l1c_continues_previous_day_grid():


Seems to test the exact same stuff as test_mag_l1c_continues_previous_day_grid so possibly could be removed?

tmplummer

Overall, looks good. Just a couple of things to think about.

tmplummer · 2026-07-10T14:35:42Z

+    logical_source = previous_day_dataset.attrs.get("Logical_source", "")
+    if isinstance(logical_source, list):
+        logical_source = logical_source[0]


Are there valid cases where Logical_source does not exist in a CDF or is a list? I have noticed that coding agents tend to put lots of this type of error handling code in where it really is not needed or useful making it mostly just bloat. I believe that it is OK to assume that the previous_day_dataset comes from a valid MAG CDF which would necessarily mean that a Logical_source attribute exists and it is a string.

tmplummer · 2026-07-10T14:38:06Z

+    if (
+        "l1b" not in logical_source
+        or "norm" not in logical_source
+        or logical_source[-1:] != sensor


Why is [-1:] used here. AFAIK, it is the same as [-1] but adds confusion.

Edit: Oh, I guess it could be for handling an empty string, but I believe that the first check would evaluate to true and so this check would get skipped.

tmplummer · 2026-07-10T15:02:07Z

+        # The inherited gap starts one period before the first in-window grid point,
+        # because interpolate_gaps only fills points strictly inside a gap. That extra
+        # leading point must not appear in the output timeline.
+        new_timeline = new_timeline[new_timeline != inherited_gap_start_ns]


I think that new_timeline > inherited_gap_start_ns and new_timeline != inherited_gap_start_ns will return the same mask in this case but I agree that using greater than is clearer for someone who doesn't know the specific logic that generated the values in new_timeline. In fact, if I am understanding correctly, new_timeline = new_timeline[1:] would also produce the same result?

sapols · 2026-07-10T20:00:26Z

Thank you both @alastairtree and @tmplummer for your reviews, this is exactly the kind of feedback I wanted! I have big shoes to fill with Maxine gone, but PRs like this are teaching me things quickly. I'll incorporate all your feedback asap. I'm at the GEM workshop all next week, but hopefully I'll find time between sessions. If not, it'll get done first thing the following week 🙏

maxineofficial and others added 3 commits July 6, 2026 11:35

Adding MAG L1C day boundary tests

485f56e

This was referenced Jul 7, 2026

MAG L1C: depend on previous day's normal-mode L1B for day-boundary time inheritance IMAP-Science-Operations-Center/sds-data-manager#1471

Open

MAG L1C day-boundary follow-ups: T018/T019, MAG validation data, and gap-management scope #3324

Open

sapols commented Jul 7, 2026

View reviewed changes

sapols requested review from alastairtree, maxineofficial and tmplummer July 7, 2026 20:26

alastairtree reviewed Jul 10, 2026

View reviewed changes

alastairtree mentioned this pull request Jul 10, 2026

Adding MAG L1C day boundary tests #3318

Draft

tmplummer reviewed Jul 10, 2026

View reviewed changes

		# Input datasets can be in any order, and are validated within mag_l1c
		if len(input_data) == 1:

		return norm_day1, norm_day2, burst_day2, meta


		def test_process_mag_l1c_continues_previous_day_grid():

Uh oh!

Conversation

sapols commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What it does

Review feedback incorporated

Tests

Scope deferred to a follow-up issue

Companion (infrastructure)

Verification

Remaining Tasks

Uh oh!

sapols commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxineofficial commented Jul 7, 2026

Uh oh!

sapols commented Jul 7, 2026

Uh oh!

sapols commented Jul 9, 2026

Uh oh!

alastairtree left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmplummer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapols commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sapols commented Jul 7, 2026 •

edited

Loading

sapols commented Jul 7, 2026 •

edited

Loading