Lo psets now take goodtimes/background information from science dependencies#3192
Lo psets now take goodtimes/background information from science dependencies#3192vineetbansal wants to merge 5 commits into
Conversation
…cies instead of ancillary files
There was a problem hiding this comment.
Pull request overview
Switches Lo PSET (L1C) processing to consume goodtimes and H/O background rates from the new L1B science dependencies (imap_lo_l1b_goodtimes, imap_lo_l1b_bgrates) instead of ancillary CSV files, removes the now-dead set_bad_times/set_bad_or_goodtimes helpers from L1B, fixes the goodtimes logical source name, and updates the CLI to load the new science dependencies. The per-ESA-step / per-spin-bin filtering that the CSV used to express via bin_start/bin_end/E-Step* columns is no longer applied — fractions/rates are broadcast across all 3600 spin bins and all ESA steps (intentional per the PR description, acknowledged loss of fidelity).
Changes:
filter_goodtimes,create_goodtimes_fraction,calculate_exposure_times, andset_background_ratesare reworked to consume xarray Datasets fromsci_dependenciesrather than DataFrames built from ancillary CSVs.set_bad_times/set_bad_or_goodtimes(and theirl1b_decall site) are removed from L1B; CLI now also pullsgoodtimesandbgratesL1B CDFs as science inputs.- Tests are reshaped accordingly (new
l1b_bgrates_dsfixture, xarray-based goodtime fixtures, simplified expectations matching the broadened filter logic).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| imap_processing/lo/l1b/lo_l1b.py | Removes set_bad_times/set_bad_or_goodtimes and their invocation from l1b_de, plus the unused pandas import. |
| imap_processing/lo/l1c/lo_l1c.py | Switches goodtimes/bgrates consumption from ancillary DataFrames to L1B xarray Datasets; simplifies signatures of filter_goodtimes, create_goodtimes_fraction, calculate_exposure_times, and set_background_rates. |
| imap_processing/cli.py | Adds goodtimes and bgrates L1B descriptors to the Lo science file list. |
| imap_processing/cdf/config/imap_lo_global_cdf_attrs.yaml | Renames Logical_source from imap_lo_l1b_good-times to imap_lo_l1b_goodtimes. |
| imap_processing/tests/lo/test_lo_l1b.py | Removes tests for the deleted bad/goodtime helpers. |
| imap_processing/tests/lo/test_lo_l1c.py | Replaces DataFrame-based fixtures with Dataset-based fixtures and rewrites expectations to match the new broadcast behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tmplummer
left a comment
There was a problem hiding this comment.
A few things to consider/change.
| goodtimes_table_df, epochs, esa_steps, spin_bins | ||
| # Keep events that fall within any goodtime window | ||
| in_goodtime = np.any( | ||
| (epochs[:, None] >= gt_starts) & (epochs[:, None] <= gt_ends), |
There was a problem hiding this comment.
What values are in l1b_de["epoch"]? Does it contain direct event times? or CCSDS packet creation times?
I just want to make sure that this mask is doing what you want it to do. It seems like filtering DE times is probably what you want, but I also don't know what times are recorded in the goodtimes_ds["gt_start/end_met"] DataArrays. Also, it is important to understand that there is some precision error in conversions to/from floating point MET to integer TTJ2000ns so you need to be careful doing these time comparisons and consider what conversions have been applied to each of the times being compared.
There was a problem hiding this comment.
I have no idea what's in l1b_de['epoch'], except that its in ttj2000ns. I didn't want to disrupt the workings of wherever that dataset is used down the line (in create_pset_counts I see..). goodtimes_ds` stores the start/end of goodtime windows in met (this was a PR approved last week).
Point noted about loss of precision - one optimization I can think of is to determine pointing_start_met/pointing_end_met directly instead of this round trip to met -> ttjns2000 -> met which is silly. I'll do that now.
Other than that if you still see a possible problem perhaps you can open an issue on this and I'll look at it next week? Thanks!
There was a problem hiding this comment.
Ok, I just double checked. It does appear that the l1b_de epoch variable contains the TTJ2000ns time of each event. That alleviates my concerns about this masking.
| pointing_goodtimes_mask = ( | ||
| (goodtimes_ds["gt_start_met"] < pointing_end_met) | ||
| & (goodtimes_ds["gt_end_met"] > pointing_start_met) | ||
| ).values |
There was a problem hiding this comment.
Is this in any way already assured in the bg_rates and goodtimes processing? I will point out that this logic will keep any goodtime interval that overhangs the pointing start or end.
Edit: Ok, I see that the overlap is computed below.
| filtered_epochs = l1b_de.sel(epoch=goodtimes_mask.astype(bool)) | ||
|
|
||
| return filtered_epochs | ||
| return l1b_de.sel(epoch=in_goodtime) |
There was a problem hiding this comment.
I noticed that below, you use the Dataset.isel(epoch=mask) method to filter. After investigating a bit, I think that using sel will work here, but it should be isel. From what I found,
If you pass a 1D boolean array to ds.isel(dim=mask), it works like numpy. However, if you pass a boolean array to ds.sel(dim=mask), it may fail or misinterpret the data if the dimension does not have integer labels that match the mask, as described in GitHub issues regarding sel behavior #4892.
There was a problem hiding this comment.
Yes I think isel is better. changed.
|
Lo team notes that the goodtimes algorithms do need some updates; Nathan will be in touch regarding that and coordinating those updates with eventually merging this PR. |
|
I also want to foreground #3021 to see if we need that before switching over to using this product |
Co-authored-by: Tim Plummer <timothy.plummer@lasp.colorado.edu>
Closes issue #3079
Change Summary
Lo
psets now take goodtimes/background information from science dependencies instead of ancillary files.Overview
The
lo_l1b_goodtimesproduct has now being producing goodtimes and H/O background rate information (the latter as thelo_l1b_bgratesproduct, auto-generated whengoodtimesis generated). This PR makes thepsetsproduct now get this information from these science dependencies instead of ancillary files.File changes
imap_processing/cdf/config/imap_lo_global_cdf_attrs.yaml- renamed logical source to match generated filename.lo_l1b.py- the functions that were trying to set good/bad times are now gone, in favor of getting this information from thegoodtimesdataset. The places in the code that created dataframes out of csvs are gone too, and now use thexr.Datasetdirectly.Note that the incoming ancillary file had hardcoded
0and59and a series of 71s as certain columns (with the generating code hardcoding these in its logic too). This implied hardcoding was not carried over when generating thebgratesdataset (the background rates are along a singleesa_levelaxis, and there's no concept ofbg_startandbg_end(bins)), but to avoid major disruptions inpsetuse, this hardcoding now shows up in lines:i.e. it is assumed that the
goodtimes_fractionused is always1.0for all ESA levsl and all (3600) spin-bins, and is now broadcast to the desired shape.Testing
Modified tests based on changes.