support corrected spin start times (use_corrected_spin_start flag) by vineetbansal · Pull Request #3548 · IMAP-Science-Operations-Center/imap_processing

vineetbansal · 2026-10-06T18:39:54Z

I've had to add the use_corrected_spin_start flag at a few more places than originally suggested, since quite a few callers outside this module are directly calling get_spin_data, and I take it that they should get the new (corrected) columns by default as well.

I haven't added any new tests, only modified the existing ones.

… True)

…est_data) has changed

vineetbansal · 2026-10-06T21:12:56Z

@tmplummer - the last commit was added because our CI cache saves the test_data/ directory (among others), which hold files packaged with the code. When a PR changes a committed file there, the cache misses and the fallback restores an older version. Only the ubuntu / Python 3.10 job uses the cache, so only that job failed.

A git checkout -- . fixes the situation.

Ideally we'd only be caching on data folders where we truly download, not containing files committed with the code, but that's for another issue.

tmplummer

Overall looks good. I am just a bit concerned about defaulting to use the corrected times. Can you do an accounting of all instruments that use these functions so we can double check where to use corrected times and where not to?

tmplummer · 2026-10-06T21:22:24Z

 @functools.cache
-def _load_spin_data_with_cache(csv_paths: tuple[Path]) -> pd.DataFrame:
+def _load_spin_data_with_cache(
+    csv_paths: tuple[Path], use_corrected_spin_start: bool


My opinion is that get_spin_data should return the full table, not one or the other spin start time. I can see instances where it would perhaps be useful to know the difference between original and corrected. For example, that was how I thought Lo prostar would apply the correct shift... Maybe that isn't true. I am open to disagreement on this.

I guess I'm confused now - I thought the purpose of introducing the *_corr columns was that no caller to the function would need to change their calls. Lo prostar (the new implementation in a pending PR) is simply calling get_spin_data(), so it will apply the correct shift once that code is merged and reprocessing is done (unless it was applying hand-crafted shifts applicable for certain date ranges, which it is not).

tmplummer · 2026-10-06T21:24:26Z

+            1,
+            0,
+            0,
+        ],


Haha, that really blew those lines up!

tmplummer · 2026-10-06T21:27:31Z



-def get_spin_data() -> pd.DataFrame:
+def get_spin_data(use_corrected_spin_start: bool = True) -> pd.DataFrame:


I'm a bit worried about defaulting to True. While I agree that it is probably the most often used default, I know for a fact that GLOWS needs this set to False. They had the assumption that the new columns would be returned as new pd.Dataframe columns and so they would not need any code changes to keep existing functionality.

By "existing functionality", do you mean that GLOWS needs the old (shifted one way or correct, depending on the date) incorrect spin data?

In that case, probably we should change its call here to pass in False?

imap_processing/imap_processing/glows/l1b/glows_l1b_data.py

Line 998 in de0d02f

spin_data = get_spin_data()

Or do they not want any code changes or re-deployment of their containers such that that line cannot change? (In which case, all other callers should pass-in True and the default changed to False?)

My concern there would be that after a while we'll simply forget to pass in True for new call sites and re-processing old dates will again generate incorrect data.

vineetbansal · 2026-10-07T18:35:16Z

@tmplummer - can you confirm if my understanding is correct:

In the sample files attached with issue #3543, I see that for DOY 169 (June 18th), the spin_start_subsec_sclk_corr is slightly ahead of spin_start_subsec_sclk. For DOY 276 (Oct 3), they're the same (and I suppose will remain so in the future).

DOY 169 was just an example, - all spin files for all dates will be reprocessed, so in all cases the *_corr columns are the correct values, for past and future dates.

tmplummer · 2026-10-07T18:45:49Z

@tmplummer - can you confirm if my understanding is correct:

In the sample files attached with issue #3543, I see that for DOY 169 (June 18th), the spin_start_subsec_sclk_corr is slightly ahead of spin_start_subsec_sclk. For DOY 276 (Oct 3), they're the same (and I suppose will remain so in the future).

DOY 169 was just an example, - all spin files for all dates will be reprocessed, so in all cases the *_corr columns are the correct values, for past and future dates.

All spin files will be reprocessed through September 30. From there on, we expect the corrected start times to match. And, yes... *_corr columns should always be the correct values.

vineetbansal · 2026-10-07T19:13:52Z

@tmplummer - perhaps what you're getting at is that certain functions should return the "best" data by default. This list slightly modified from your original get_spin_number, get_spin_angle, get_spin_angle, get_instrument_spin_phase in the issue because get_spin_angle is repeated + is not affected by this change (because it relies on the caller to pass in the spin-phases anyway):

get_spin_number
get_spacecraft_spin_phase
get_instrument_spin_phase

and the functions that return DataFrames should conform to their old behavior, only adding columns, not changing them:

get_spin_data
interpolate_spin_data

Even though no code is doing any checks across these corrected/uncorrected columns, that allows us (or the instrument teams) to do that in the future. Code that is looking inside the dataframes will get uncorrected values till modified.

Can you give this a thumbs-up if you want this?

… along with new ones

tmplummer · 2026-10-07T20:37:01Z

Can you give this a thumbs-up if you want this?

I think so... do you agree?

Then I would add that we should look at all uses of the functions in spin.py and try to determine which instruments don't want the "best" data.

tmplummer · 2026-10-07T21:29:48Z

This table was generated using AI a while back. It summarizes the instruments that rely on spin-table data.

Instrument	Not fixable by reprocessing (needs code update)	Fixable by reprocessing (dependency noted)	Other spin-table use (non-phase)	Unaffected
IMAP-Hi	L1A `SCI_CNT` histogram	L1B/L1C/L2 DE pipeline (spin-table + CK); Goodtimes (spin-table only)	—	HK/DIAG/MEMDMP
IMAP-Lo	L1B `histrates`/`monitorrates`, all L2 ENA sky maps, `prostar`	L1B DE/`derates`, L1C PSET counts channel (spin-table + CK)	`histrates`/`monitorrates`' `spin_cycle` bookkeeping index and L1B `badtimes` (`thruster_firing` flag) — both spin-table only; L1C PSET `start_spin_number`/`end_spin_number` metadata — spin-table only	(none — see `prostar` note)
IMAP-Ultra	Onboard angle-histogram APIDs (not currently consumed — latent); sector-mode dead-time calibration curve (minor)	DE pipeline, PSET counts, L2 maps (spin-table + CK); Goodtimes/badtimes/extendedspin, PSET exposure/GF/efficiency (spin-table only)	—	Helio-PSET exposure/GF geometry (nominal simulated kernels, not real attitude)
CoDICE	All sectored/directional products (hi-sectored, lo-counters, priority buffers, direct events), I-ALiRT CoDICE-Hi quicklook	none	none	Omnidirectional products (hi-omni, hi-priority, hi-counters-aggregated/singles)
HIT	`sectored-rates` → `macropixel-intensity`	none	none	`standard-rates`/`summed-rates` and their L2 intensities
GLOWS	—	UV photon histogram bin-to-sky mapping (spin-table, + CK for L2 ecliptic projection) — via the existing `spin_offset_correction` ancillary parameter, pending one verification (see GLOWS section)	`spin_period_ground_average`/`std_dev` diagnostic (spin-table only)	Direct Events (individually timestamped, no ground spin-phase computed yet)
IDEX	none	All products, L1B/L2A/L2B/L2C (spin-table + CK)	—	—
MAG	none	L2 despin (CK only); L1D despin + spin-tone correction + spin-offsets ancillary product (CK + spin-table)	L1D uses median spin-table `spin_period_sec` for gap estimation	L1A/L1B/L1C (raw instrument-frame vectors)
SWE	none	L2 spin/angle binning of flux & PSD (spin-table only)	—	L1A/L1B (`spin_sector` is a sequential sample index, not a real angle)
SWAPI	none	none	none	All products
I-ALiRT	CoDICE-Hi quicklook (see CoDICE); real-time MAG geolocation & spin fields (bypasses spin-table/CK entirely — uses telemetered onboard star-tracker attitude directly, no ground-truth substitution path exists)	—	—	Final ECLIPJ2000→GSE/GSM/RTN step (ephemeris/SPK-based)
`spacecraft/`	—	`pointing-attitude` descriptor is the CK-regeneration step used to fix this anomaly	—	`quaternions` product (raw diagnostic telemetry)
`ena_maps` (shared L2 lib, used by Hi/Lo/Ultra/IDEX)	Inherits Lo's status for Lo maps	Inherits Hi/IDEX/Ultra's status (CK, live inside this module for Ultra)	—	Ephemeris (SPK)-based position/velocity annotation, Compton-Getting/ram-mask logic

…olate_spin_data; overridable

vineetbansal · 2026-10-08T13:49:47Z

@tmplummer - the behavior that makes sense to me and that I've gone with based on the discussions above:

All functions with use_corrected_spin_start flag have it set to True by default.
- get_spin_data -> DataFrame
- interpolate_spin_data -> DataFrame
- get_spin_number
- get_spacecraft_spin_phase
- get_instrument_spin_phase
With this default:
- The callers don't need to change and do not need to know that anything happened under the hood (i.e. addition of 3 new *_corr columns).
- For functions that return DataFrames, the new *_corr columns are available under the old names, and thus no code change is needed in the callers.

This is somewhat motivated by looking at the number of callers who are calling get_spin_data() and then doing calculations with spin_start_met columns etc. (also reflected in the large AI-generated list above). I would think they should start picking up the corrected values automatically, and only exceptions like GLOWS should need to change.

Passing in use_corrected_spin_start=False to functions that return DataFrame makes the *_corr columns available, without changing the uncorrected columns.
- Callers like GLOWS code are also indeed calling get_spin_data() and reaching in for spin_start_met - these can easily be changed to get_spin_data(use_corrected_spin_start=False) to make explicit that they want uncorrected data for pre-Sept-30th dates. Their code that deals with spin_start_met can remain unchanged (or they can do fancy stuff with spin_start_met vs spin_start_met_corr if they want).
- Same for other functions - if GLOWS was calling get_spin_number(), it should now call get_spin_number(use_corrected_spin_start=False) if it wants the uncorrected spin number for a MET.

Hope this sounds reasonable. I haven't changed any calling code for any instrument in the PR (yet), because I don't know what each instrument team is expecting to happen. Perhaps this can be ironed out in the meeting today.

leowerneck · 2026-10-08T15:26:06Z

@vineetbansal Margaret just got confirmation from the GLOWS team that the desired behavior is to reprocess everything with the uncorrected spins, i.e., with use_corrected_spin_start = True.

Do we need a separate issue for L3? I can work on that if there isn't one already. Should be a couple of lines diff.

vineetbansal · 2026-10-08T15:53:31Z

@leowerneck - I think you meant use_corrected_spin_start = False. Yes, I think the L3 code should be handled in its own issue. I'm not sure how it specifies version numbers of imap_processing that it needs, but this flag doesn't exist in these functions before this PR, just FYI.

@tmplummer - I see two places where it is interfacing with spin.py, so the flag needs to be passed-in at 2 places. The last commit in this PR makes those changes.

GLOWS seems to be doing something like this in their ancillary file.

leowerneck · 2026-10-08T16:18:16Z

It seems like L3 does not need modification. For example, running:

grep -rnE "spice\.spin|get_spin_|spin_phase\(|interpolate_spin_data" imap_l3_processing

doesn't return anything obvious. The only hit is get_spin_angle_index, which doesn't need modification AFAICT.

tmplummer

Looks good! Thanks for taking this on.

support corrected spin start times (use_corrected_spin_start, default…

529535b

… True)

vineetbansal requested a review from tmplummer October 6, 2026 18:40

not using stale data when a packaged data file (e.g. in tests/spice/t…

05be134

…est_data) has changed

tmplummer reviewed Oct 6, 2026

View reviewed changes

get_spin_data and interpolate_spin_data return their original columns…

31681b3

… along with new ones

use corrected spin start times by default in get_spin_data and interp…

53093a0

…olate_spin_data; overridable

GLOWS is using uncorrected data from spin.py

0c750f8

mmshaw mentioned this pull request Oct 8, 2026

Update instrument calls to spin related functions #3470

Closed

tmplummer approved these changes Oct 8, 2026

View reviewed changes

tmplummer merged commit fc4e121 into IMAP-Science-Operations-Center:dev Oct 8, 2026
22 checks passed

+,
+,
+,
+                      ],



		def get_spin_data() -> pd.DataFrame:
		def get_spin_data(use_corrected_spin_start: bool = True) -> pd.DataFrame:

Conversation

vineetbansal commented Oct 6, 2026

Uh oh!

vineetbansal commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmplummer left a comment

Choose a reason for hiding this comment

Uh oh!

tmplummer Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

vineetbansal Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

tmplummer Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

tmplummer Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

vineetbansal Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tmplummer commented Oct 7, 2026

Uh oh!

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

Uh oh!

Uh oh!

tmplummer commented Oct 7, 2026

Uh oh!

tmplummer commented Oct 7, 2026

Uh oh!

vineetbansal commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leowerneck commented Oct 8, 2026

Uh oh!

vineetbansal commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leowerneck commented Oct 8, 2026

Uh oh!

tmplummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vineetbansal commented Oct 6, 2026 •

edited

Loading

vineetbansal commented Oct 7, 2026 •

edited

Loading

vineetbansal commented Oct 7, 2026 •

edited

Loading

vineetbansal commented Oct 8, 2026 •

edited

Loading

vineetbansal commented Oct 8, 2026 •

edited

Loading