Skip to content
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

Limit stat update range to end of AOPCADMD data #318

Merged
merged 3 commits into from
Mar 24, 2025

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Mar 20, 2025

Description

Limit stat update range to end of AOPCADMD data by default.

Fixes issue that more kadi.obsid events from sot/kadi#343 could cause errors out of the update code.

Interface impacts

Testing

Unit tests

  • Linux
ska3-jeanconn-fido> git rev-parse HEAD
69ec2ba4cf5262f233110b7a379bbd0c7a49776e
ska3-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: anyio-4.7.0, timeout-2.3.1
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 ......................................... [ 64%]
.....                                                                    [ 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
  /proj/sot/ska3/flight/lib/python3.12/pty.py:95: DeprecationWarning: This process (pid=2840731) 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
  /proj/sot/ska3/flight/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 549.75s (0:09:09) ==================

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

For functional testing, I created a ska3 test environment with the updated kadi events3.db3 file seeded with the flight versions of the acq and guide stats tables and ran masters mica status update jobs on that and then re-seeded the tables and ran this PRs update jobs on on those tables.

Unfortunately, with the data available just now, there are no errors from the masters version - I believe this is due to the fact that there is already code to not process the obsids if 1) there is no obspar available or 2) there is no complete next manvr. So we'd need a bunch of events with obspars but no cxc aopcadmd data to hit this new filter in the new PR.

However, given the scope of this PR I think just showing that the update job processes without error using the new code is really sufficient functional testing:

In [1]: run /fido.real/miniforge3/envs/ska3-latest/share/mica/update_acq_stats.py
Processing obsid 29979
Found obsid manvr at 2025:082:14:11:37.737
Found dwell at 2025:082:14:49:35.800
calculating statistics
arranging stats into tabular data
saving stats to h5 table
Processing obsid 42541
Skipping obsid 42541: No manvr or dwell for 42541
Processing obsid 28111
Found obsid manvr at 2025:082:19:03:29.863
Found dwell at 2025:082:19:32:29.801
calculating statistics
arranging stats into tabular data
saving stats to h5 table
Processing obsid 28357
Found obsid manvr at 2025:082:23:17:16.240
Found dwell at 2025:082:23:55:51.202
calculating statistics
arranging stats into tabular data
saving stats to h5 table

In [2]: run /fido.real/miniforge3/envs/ska3-latest/share/mica/update_guide_stats.py
Processing obsid 29979
Found obsid manvr at 2025:082:14:11:37.737
Found dwell at 2025:082:14:49:35.800
calculating statistics
arranging stats into tabular data
saving stats to h5 table
Processing obsid 42541
Skipping obsid 42541: No manvr or dwell for 42541
Processing obsid 28111
Found obsid manvr at 2025:082:19:03:29.863
Found dwell at 2025:082:19:32:29.801
calculating statistics
arranging stats into tabular data
saving stats to h5 table
Processing obsid 28357

@jeanconn
Copy link
Contributor Author

@taldcroft I'm not sure how well this generalizes or if it will be perfect, but what are your thoughts on this approach in the mica guide/acq stats code to not try to process obsid events that are from maude but aren't yet in cxc archive? I'm using the fancy kadi stop filter because I'd prefer the "obsid" event (for whatever it is worth) to be fully contained.

@taldcroft
Copy link
Member

I did a drive-by earlier and it took me a second to understand the driver for stop__lte, but it does make sense and I agree with the approach. There are a few things that are not immediately obvious:

  • The obsid event interval is the full interval of constant obsid.
  • Obsid commanding is normally in the maneuver between observations. This means that a kalman interval is fully contained within an obsid event.
  • The normal stop kwarg of filter is inclusive, so that is that you need stop__lte to ensure that the desired stop is after the obsid stop.

Some facsimile of the above should probably go in a code comment.

One question I did have here is why the original processing logic used obsids instead of manvrs. Even after SCS-107, a manvr maps one-to-one with observations, while obsids do not. I admit I haven't looked at the code outside the changes so I could be missing something.

@jeanconn
Copy link
Contributor Author

Right, I wasn't sure how much to comment on gotchas of kadi events in this PR. For example it looks to me like the "stop" keyword is both inclusive and filters on the start time of the obsid event, unless the extended django syntax is used in which case it looks to apply to the stop attribute of the obsid event. I don't know if that's documented someplace or if I'm just getting confused by some spot checks on behavior.

@jeanconn
Copy link
Contributor Author

With regard to obsids instead of kadi manvrs, I don't think it really matters - I think this code predates having the obsid available in maneuver events for example, and since it is all obsid-based anyway, just getting obsids to process made sense when this was plugged in here as a replacement for data from one of the sybase tables.

@jeanconn jeanconn marked this pull request as ready for review March 24, 2025 19:32
@jeanconn
Copy link
Contributor Author

With what we currently have in the kadi-events-from-maude data, I couldn't see a slam dunk here in functional testing. I think this is still a benign fix that can just go in - or we could skip the PR if the error condition is actually not likely to occur often. Either way.

@jeanconn jeanconn requested review from javierggt and taldcroft March 24, 2025 19:34
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.

Looks good!

@jeanconn jeanconn merged commit 73e2451 into master Mar 24, 2025
1 of 2 checks passed
@jeanconn jeanconn deleted the limit-stat-update-range branch March 24, 2025 20:12
This was referenced Mar 24, 2025
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