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

Improve pitch/roll_comp, logical_intervals, state_intervals, get_ofp_states, get_telem_table #249

Merged
merged 9 commits into from
Apr 23, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Apr 8, 2023

Description

This is a hodge-podge of related improvements to utilities in cheta. The initial driver was a problem in [aca] Kadi commands state validation: ALERT on Apr 1 ending with:

 File "/proj/sot/ska3/flight/lib/python3.10/site-packages/cheta/utils.py", line 334, in state_intervals
    raise ValueError("Filtered data length must be at least 2")
ValueError: Filtered data length must be at least 2

Generally speaking the original implementation of pitch_comp and roll_comp was not robust. It depended on get_ofp_states in kadi.utils which was also not robust. So this turned into an effort to revisit this code and make it work under most circumstances. In particular dealing with short query intervals was a focus.

Since the code hinges on state_intervals and logical_intervals, those got pulled into the effort.

This is used by sot/kadi#285.

Changes

  • Fix pitch_comp and roll_comp to work for short query intervals like normal MSIDs, namely to return values if the interval covers a time where the relevant primary telemetry are available. Since this computation uses telemetry sampled from 1.025 sec up to 5 minutes (ephemeris), this is not completely trivial. In any case it should always return a result, even if that result has no data. Added new tests of this.
  • Add start and stop args to logical_intervals and state_intervals. This is useful when you want the table to states to exactly span the requested interval. This is different from the default behavior where the states start and end times are defined by the input times.
  • For logical_intervals and state_intervals provide special handling for cases with input data of length 0 and 1. Previously you would get some non-obvious exceptions.
  • Add the get_telem_table function which was migrated and generalized somewhat from the kadi validate code.

TODO:

  • Add more tests for state_intervals

Fixes #247

Interface impacts

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Independent check of unit tests by Jean

  • Linux (fido)

Functional tests

No functional testing.

@jeanconn
Copy link
Contributor

Will this also close #247 ?

@taldcroft
Copy link
Member Author

Will this also close #247 ?

Yes! I've updated the description.

@taldcroft taldcroft requested review from javierggt and jeanconn April 19, 2023 13:47
@jeanconn
Copy link
Contributor

This has an open TODO of "Add more tests for state_intervals". Should that get cut or should I review in advance of that (if it is ongoing work)?

cheta/derived/comps.py Outdated Show resolved Hide resolved
@jeanconn
Copy link
Contributor

I'm seeing test failures on kady

============================================= short test summary info ==============================================
FAILED cheta/tests/test_intervals.py::test_logical_intervals_start_stop_1 - AssertionError: assert ['duration ts...   3.5   4.5'] == ['duration ts...   3.5   6.0']
FAILED cheta/tests/test_intervals.py::test_logical_intervals_start_stop_2 - AssertionError: assert ['duration ts...   3.5   4.5'] == ['duration ts...   3.5   6.0']
=================================== 2 failed, 76 deselected, 9 warnings in 3.02s ===================================
ska3-jeanconn-kady> 

Does this need to be tested with another change?

@taldcroft
Copy link
Member Author

About the TODO, I did that and it is now checked.

cheta/derived/comps.py Outdated Show resolved Hide resolved
@taldcroft
Copy link
Member Author

About the test failure, I'm seeing that now on Mac in my branch. No idea when that broke and why I didn't see it before. Hence why we have that checkbox for someone else to run the tests at the end.

@taldcroft taldcroft force-pushed the better-pitch-roll-comp-and-utils branch from 96f63d5 to 4dbed06 Compare April 20, 2023 17:37
@taldcroft taldcroft force-pushed the better-pitch-roll-comp-and-utils branch from 4dbed06 to 4d12f7b Compare April 20, 2023 17:40
@taldcroft
Copy link
Member Author

@jeanconn - I think I have resolved all the issues with a code fix and new tests and a rebase.

@jeanconn
Copy link
Contributor

Looks like this just needs author to re-check one of the unit test checkboxes.

@taldcroft taldcroft merged commit ff8869d into master Apr 23, 2023
@taldcroft taldcroft deleted the better-pitch-roll-comp-and-utils branch April 23, 2023 09:36
@javierggt javierggt mentioned this pull request Jul 12, 2023
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.

pitch_comp roll_comp computed msids fail
2 participants