Drop invalid timestamps in GLOWS L1B#3139
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a safeguard in the GLOWS L1B histogram pipeline to skip histogram epochs with invalid timing metadata (imap_start_time == 0.0), preventing downstream SPICE spin-phase lookups from failing on old/bad L1A inputs (Issue #3007).
Changes:
- Add filtering logic in
process_histogram()to drop epochs whereimap_start_timeis zero and emit a warning with the number dropped. - Update the histogram test fixture to use non-zero
imap_start_timeby default. - Add a unit test that injects zero start times and verifies the filtered result length.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
imap_processing/glows/l1b/glows_l1b.py |
Filters out imap_start_time==0.0 epochs during histogram processing and logs the drop count. |
imap_processing/tests/glows/test_glows_l1b.py |
Adjusts histogram fixture timestamps and adds a test validating the new filtering behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| invalid_mask = l1a["imap_start_time"].values == 0.0 | ||
| if invalid_mask.any(): | ||
| logger.warning( | ||
| "GLOWS L1B: Skipping %d histogram(s) with imap_start_time=0.0 " | ||
| "(invalid timing data) at epochs: %s", | ||
| invalid_mask.sum(), | ||
| l1a["epoch"].values[invalid_mask], | ||
| ) | ||
| l1a = l1a.isel(epoch=~invalid_mask) | ||
|
|
There was a problem hiding this comment.
This would be good to double check
There was a problem hiding this comment.
Ah yes, I will submit a fix for this
| logger.warning( | ||
| "GLOWS L1B: Skipping %d histogram(s) with imap_start_time=0.0 " | ||
| "(invalid timing data) at epochs: %s", | ||
| invalid_mask.sum(), | ||
| l1a["epoch"].values[invalid_mask], | ||
| ) |
There was a problem hiding this comment.
Just curious, do we have log limits?
There was a problem hiding this comment.
Nah, we should be good. I mean, I'm sure there is a limit eventually, but for the number of times in each file this should not be an issue.
| output = process_histogram( | ||
| hist_dataset, | ||
| mock_ancillary_exclusions, | ||
| mock_ancillary_parameters, | ||
| pipeline_settings, | ||
| ) | ||
| # 2 invalid epochs dropped; 18 valid epochs remain | ||
| assert output[0].sizes["epoch"] == 18 |
There was a problem hiding this comment.
This is worth considering - a test like this would have caught the other issue you fixed where the output dataset was using the original input_dataset["epoch"] rather than the valid epochs
| output_dataarrays, input_dataset["epoch"], input_dataset["bins"], cdf_attrs | ||
| output_dataarrays, | ||
| output_dataarrays[0].coords["epoch"], | ||
| input_dataset["bins"], |
There was a problem hiding this comment.
does input_dataset["bins"] share the same size as the input_dataset["epoch"]?
There was a problem hiding this comment.
This is a good catch. Let me get the bins from the same place as the epoch.
There was a problem hiding this comment.
Actually, this is a coordinate, not a variable. So, it's independent of epoch. We should be good as-is.
vmartinez-cu
left a comment
There was a problem hiding this comment.
I added a couple minor non-blocking comments. Nice job on this!
06b4a95
into
IMAP-Science-Operations-Center:dev
Change Summary
Addresses #3007. This isn't strictly required since the L1A processing drops start times anyway, but this fixes the problem for any old L1A files that might still be hanging around.
This does complete #3007 and unblocks #2900.
Overview
This code filters out any timestamps that are zero, and logs out how many were filtered.
File changes
Testing
There is a test to confirm that the filter works correctly.