Fix bug in acq/guide stats processing range #320
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the acquisition and guide stats processing range by ensuring that the filtering of obsids uses the correct type for the stop time argument, and it removes legacy pause code from the acquisition stats update module.
- In update_guide_stats.py, the filter call now converts stop to a date string using CxoTime(stop).date.
- In update_acq_stats.py, the filter call has been similarly updated and the code that paused processing during a specific hour has been removed.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| mica/stats/update_guide_stats.py | Updated obsids filter to convert stop to a date string. |
| mica/stats/update_acq_stats.py | Updated obsids filter similarly; removed legacy pause logic. |
Comments suppressed due to low confidence (2)
mica/stats/update_guide_stats.py:422
- Ensure that converting stop to a date using CxoTime(stop).date returns the format expected by events.obsids.filter. Verify that this change fully addresses the bug caused when passing non-string Chandra seconds.
start=last_tstart, stop__lte=CxoTime(stop).date
mica/stats/update_acq_stats.py:488
- Confirm that the conversion from aopcadmd_end_time to a date string using CxoTime(aopcadmd_end_time).date meets the requirements for filtering in events.obsids.filter and fixes the reported bug.
start=last_tstart, stop__lte=CxoTime(aopcadmd_end_time).date
There was a problem hiding this comment.
This looks OK.
Note that fetch.get_time_range() has a format option to specify the output format. So you could have done that instead of repeating code.
And it's worth remembering the pattern of coercing a CxoTimeLike argument or variable (like stop) to CxoTime near the top of a function, instead of doing that piecemeal later on.
|
"Note that fetch.get_time_range() has a format option" thanks! Obviously I'd forgotten about that format option and forgot to check the help. |
Description
Fix bug in acq/guide stats processing range
Also removes some old code that was intended to pause acquisition statistics if run during the hour that the kadi update process was supposed to run. Now that code makes no sense as kadi update runs every ten minutes.
This PR fixes a bug I introduced in #318 where I assumed that when using an event filter that the keyword arg to stop__lte could be basically CxoTimeLike. That's not true. It looks like the filter will work if supplied a string or object, but doesn't work if given Chandra seconds.
Interface impacts
Testing
Unit tests
Independent check of unit tests by [REVIEWER NAME]
Functional tests
No functional testing.
I have not tested removing the code that added the 3720 second wait. I think this is fine by inspection.