2815 bug hi l1c goodtimes#2818
2815 bug hi l1c goodtimes#2818subagonsouth wants to merge 7 commits intoIMAP-Science-Operations-Center:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Hi “qualified event” selection so statistical filters (and shared utilities) require events to pass both coincidence-type and TOF-window criteria, aligning goodtimes behavior with L1C PSET counting.
Changes:
- Refactors “qualified event” logic into shared utilities (
iter_qualified_events_by_config,compute_qualified_event_mask,get_tof_window_mask) and reuses them inhi_l1c.pyandhi_goodtimes.py. - Updates goodtimes statistical filters to consume precomputed qualified-event masks (and corrects Filter 2’s event dimension usage to
event_met). - Updates/extends tests, including a new Filter 2 test ensuring only qualified events contribute to cluster detection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
imap_processing/hi/utils.py |
Adds shared utilities for coincidence + TOF-window qualification and related helpers. |
imap_processing/hi/hi_l1c.py |
Replaces per-row qualification logic with shared iter_qualified_events_by_config generator in pset_counts(). |
imap_processing/hi/hi_goodtimes.py |
Precomputes qualified-event masks and wires them into Statistical Filters 1 & 2 (including event_met dim fix in Filter 2). |
imap_processing/tests/hi/test_hi_l1c.py |
Updates tests to use new shared utils and dict-based TOF windows. |
imap_processing/tests/hi/test_hi_goodtimes.py |
Updates tests to pass qualified masks and adds cluster test guarding against unqualified-event inclusion. |
imap_processing/cli.py |
Only requires repointing for Hi processing when data_level != "l2". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| for esa_energy, config_row, qualified_mask in iter_qualified_events_by_config( | ||
| de_ds, config_df, esa_energy_steps | ||
| ): |
There was a problem hiding this comment.
Can you take this out iter_qualified_events_by_config( de_ds, config_df, esa_energy_steps ) and assign to a variable and then use in the for loop? It may help with performance.
imap_processing/hi/utils.py
Outdated
| if tof_fill_vals is None: | ||
| tof_fill_vals = {} |
There was a problem hiding this comment.
I don't know enough here. So sorry if I get this wrong. You are not returning at here because you can get mask with (low, high) bit operation?
imap_processing/hi/utils.py
Outdated
| if tof_var in de_ds: | ||
| tof_fill_vals[tof_var] = de_ds[tof_var].attrs.get("FILLVAL", np.nan) |
There was a problem hiding this comment.
Just curious. Why is there case when tof_var is not in the DE? is that because you can not detect some of those pairs sometimes in a day?
There was a problem hiding this comment.
Good catch. It should always be present and if it were missing, this should raise an error.
| qualified_mask : np.ndarray | ||
| Boolean mask where True = event qualifies for this (esa, cal_prod). | ||
| """ | ||
| n_events = len(de_ds["event_met"]) if "event_met" in de_ds.dims else 0 |
There was a problem hiding this comment.
Is there reason to continue if no event is found?
|
|
||
| for config_row in esa_df.itertuples(): | ||
| if n_events == 0 or not np.any(esa_mask): | ||
| yield esa_energy, config_row, np.zeros(n_events, dtype=bool) |
There was a problem hiding this comment.
I haven't used yield enough. I read that it's returning to the caller function if no event_was found or esa_mask. Is that true?
There was a problem hiding this comment.
Now I am wondering if you called iter_qualified_events_by_config() function in the for loop for this reason.
There was a problem hiding this comment.
Yes, it is intended to be used in a for loop.
tech3371
left a comment
tech3371
left a comment
There was a problem hiding this comment.
I haven't found anything major. LGTM!
Summary
Fixes a bug where statistical filters 1 and 2 in hi_goodtimes.py only checked coincidence_type when identifying "qualified" events. They now also check TOF window values, matching the logic in hi_l1c.py:pset_counts().
Changes
Closes: #2815