Skip to content

Fix bug in acq/guide stats processing range #320

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 7, 2025
Merged

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Apr 21, 2025

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.

In [2]: obsids1 = events.obsids.filter(start="2025:104", stop__lte=CxoTime("2025:105").secs)
   ...: obsids1[len(obsids1) - 1]
Out[2]: <Obsid: start=2025:111:07:41:08.907 dur=5720 obsid=42463>

In [3]: obsids2 = events.obsids.filter(start="2025:104", stop__lte="2025:105")
   ...: obsids2[len(obsids2) - 1]
Out[3]: <Obsid: start=2025:104:19:47:55.220 dur=14879 obsid=27993>

In [4]: obsids3 = events.obsids.filter(start="2025:104", stop__lte=CxoTime("2025:105"))
   ...: obsids3[len(obsids3) - 1]
Out[4]: <Obsid: start=2025:104:19:47:55.220 dur=14879 obsid=27993>

In [5]: obsids4 = events.obsids.filter(start="2025:104", stop__lte=CxoTime("2025:105").date)
   ...: obsids4[len(obsids4) - 1]
Out[5]: <Obsid: start=2025:104:19:47:55.220 dur=14879 obsid=27993>

Interface impacts

Testing

Unit tests

  • Linux
================================================= 1 passed in 9.30s ==================================================
(ska3-latest) jeanconn-fido> git rev-parse HEAD
62901d156b3d3e7511938f6b1673eac1ec899d7c
(ska3-latest) jeanconn-fido> pytest
================================================ test session starts =================================================
platform linux -- Python 3.12.8, pytest-8.3.4, pluggy-1.5.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: timeout-2.3.1, anyio-4.7.0
collected 113 items                                                                                                  

mica/archive/tests/test_aca_dark_cal.py ..................                                                     [ 15%]
mica/archive/tests/test_aca_hdr3.py ..                                                                         [ 17%]
mica/archive/tests/test_aca_l0.py .....                                                                        [ 22%]
mica/archive/tests/test_asp_l1.py .......                                                                      [ 28%]
mica/archive/tests/test_cda.py ..............................................                                  [ 69%]
mica/archive/tests/test_obspar.py .                                                                            [ 69%]
mica/report/tests/test_report.py ..                                                                            [ 71%]
mica/report/tests/test_write_report.py .                                                                       [ 72%]
mica/starcheck/tests/test_catalog_fetches.py ...............                                                   [ 85%]
mica/stats/tests/test_acq_stats.py ...                                                                         [ 88%]
mica/stats/tests/test_guide_stats.py ....                                                                      [ 92%]
mica/vv/tests/test_vv.py .........                                                                             [100%]

==================================================warnings summary ==================================================
mica/mica/archive/tests/test_asp_l1.py::test_update_l1_archive
  /fido.real/miniforge3/envs/ska3-latest/lib/python3.12/pty.py:95: DeprecationWarning: This process (pid=922524) is multi-threaded, use of forkpty() may lead to deadlocks in the child.
    pid, fd = os.forkpty()

mica/mica/archive/tests/test_cda.py::test_get_proposal_abstract
mica/mica/archive/tests/test_cda.py::test_get_proposal_abstract
mica/mica/report/tests/test_write_report.py::test_write_reports
mica/mica/report/tests/test_write_report.py::test_write_reports
  /fido.real/miniforge3/envs/ska3-latest/lib/python3.12/site-packages/bs4/builder/_lxml.py:124: DeprecationWarning: The 'strip_cdata' option of HTMLParser() has never done anything and will eventually be removed.
    parser = parser(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
==================================== 113 passed, 5 warnings in 551.31s (0:09:11)

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

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.

@jeanconn jeanconn marked this pull request as ready for review April 22, 2025 11:53
@jeanconn jeanconn requested a review from Copilot April 22, 2025 11:53
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jeanconn jeanconn requested review from javierggt and taldcroft April 22, 2025 16:20
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeanconn
Copy link
Contributor Author

jeanconn commented May 6, 2025

"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.

@jeanconn jeanconn merged commit fcffe2c into master May 7, 2025
2 checks passed
@jeanconn jeanconn deleted the bugfix-stats-range branch May 7, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants