2822 hi l1b goodtimes implement bad tdc cal culling algorithm#2835
Conversation
There was a problem hiding this comment.
Pull request overview
Adds IMAP-Hi goodtimes culling based on TDC calibration status from L1A DIAG_FEE data, wiring the new dependency through the CLI processing path and expanding tests to cover the new filter and updated call signature.
Changes:
- Extend
hi_goodtimes()/_apply_goodtimes_filters()to accept L1A DIAG_FEE and apply a newmark_bad_tdc_cal()filter. - Update CLI Hi L1B goodtimes processing to require/load a DIAG_FEE CDF and pass it into
hi_goodtimes(). - Add comprehensive unit tests for
mark_bad_tdc_cal()and update existing CLI/goodtimes tests for the new dependency and argument order.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
imap_processing/hi/hi_goodtimes.py |
Adds mark_bad_tdc_cal, threads DIAG_FEE through goodtimes orchestration, and updates filter ordering/docs. |
imap_processing/cli.py |
Fetches/validates a single L1A DIAG_FEE dependency for Hi goodtimes and passes the loaded dataset into hi_goodtimes(). |
imap_processing/tests/test_cli.py |
Updates Hi goodtimes CLI test to include DIAG_FEE input and validate the new hi_goodtimes() call signature. |
imap_processing/tests/hi/test_hi_goodtimes.py |
Adds new test suite for mark_bad_tdc_cal and updates existing goodtimes tests to include the DIAG_FEE dependency. |
💡 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.
imap_processing/hi/hi_goodtimes.py
Outdated
| # Based on sample code in culling_v2.c, skip this check if we have fewer | ||
| # than two diag_fee packets. | ||
| if len(diagfee.epoch) < 2: | ||
| logger.warning("No DIAG_FEE data to use for selecting good times") |
There was a problem hiding this comment.
The early-return check uses len(diagfee.epoch) < 2 but the warning message says "No DIAG_FEE data...". When there is exactly 1 packet this message is misleading; consider updating it to indicate "insufficient DIAG_FEE packets" (and optionally include the count) to aid debugging.
| logger.warning("No DIAG_FEE data to use for selecting good times") | |
| logger.warning( | |
| "Insufficient DIAG_FEE packets to select good times (found %d, need at least 2)", | |
| len(diagfee.epoch), | |
| ) |
imap_processing/hi/hi_goodtimes.py
Outdated
| # Check TDC calibration status (bit 1: 1=good, 0=bad) | ||
| any_tdc_failed = False | ||
|
|
||
| if check_tdc1 and (diagfee["tdc1_cal_ctrl_stat"].values[i] & 2) == 0: |
There was a problem hiding this comment.
Curious why you are & with 2. Can you add comment about it?
There was a problem hiding this comment.
This is a binary and operation. I think that the comment "(bit 1: 1=good, 0=bad)" explains it?
tech3371
left a comment
There was a problem hiding this comment.
Thank you for refactoring. It's easier to follow!
Co-authored-by: Tenzin Choedon <36522642+tech3371@users.noreply.github.com>
8fc4fa8
into
IMAP-Science-Operations-Center:dev
Summary
Details
mark_bad_tdc_cal Algorithm
Based on C reference (drop_bad_tdc_diagfee in culling.c):
Filter Order Update
Goodtimes filters now apply in order:
Closes: #2822