Improve command states with better transition infrastructure #340

taldcroft · 2024-10-27T11:03:37Z

Description

Improve code and processing of state transitions.

In particular provide the infrastructure to allow state-dependent filtering of scheduled callback transitions. The real-life test case here is a safe mode that occurred during a maneuver. The current code ends up applying all the scheduled transitions so that the attitude bounces around and there is a transition to pcad_mode = NPNT in the safe mode. This PR fixes that.

Details:

Add Transition class which is used for every transition. This inherits from dict and includes date and state_constraints attributes.
Add TransitionCallback class for callbacks. Previously this was handled by setting the state value to a function.
This sometimes required use of functools.partial and things got a bit awkward.
Use modern __init_subclasses__ method instead of a metaclass to populate the global states registries.

Interface impacts

Testing

Unit tests

Mac

(ska3) ➜  kadi git:(better-transitions) git rev-parse HEAD 
02ca6b2619be027a02bf190546bbb5c0ac7508a6
(ska3) ➜  kadi git:(better-transitions) env PYTHONPATH=/Users/aldcroft/git/ska_sun:/Users/aldcroft/git/ska_helpers pytest              
================================================= test session starts =================================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: cov-5.0.0, timeout-2.2.0, anyio-4.3.0
collected 182 items                                                                                                   

kadi/commands/tests/test_commands.py .......................................................................... [ 40%]
........                                                                                                        [ 45%]
kadi/commands/tests/test_states.py .......................x.........................                            [ 71%]
kadi/commands/tests/test_validate.py ...................                                                        [ 82%]
kadi/tests/test_events.py ..........                                                                            [ 87%]
kadi/tests/test_occweb.py ......................                                                                [100%]

===================================== 181 passed, 1 xfailed in 117.44s (0:01:57) ======================================

Independent check of unit tests by Jean

Linux

(ska3-masters) jeanconn-fido> pytest
================================================================== test session starts ===================================================================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 182 items                                                                                                                                      

kadi/commands/tests/test_commands.py ..................................................................................                            [ 45%]
kadi/commands/tests/test_states.py .......................x.........................                                                               [ 71%]
kadi/commands/tests/test_validate.py ...................                                                                                           [ 82%]
kadi/tests/test_events.py ..........                                                                                                               [ 87%]
kadi/tests/test_occweb.py ......................                                                                                                   [100%]

======================================================= 181 passed, 1 xfailed in 216.31s (0:03:36) =======================================================
(ska3-masters) jeanconn-fido> git rev-parse HEAD
02ca6b2619be027a02bf190546bbb5c0ac7508a6

Functional tests

No functional testing.

jeanconn · 2024-10-29T19:02:30Z

I'm surprised that this ended up with conflicts.

kadi/commands/states.py

jeanconn · 2024-10-29T19:33:15Z

Overall, this feels like a large change so I'm just trying to puzzle out if we need any additional functional testing or if the module tests are more than enough.

taldcroft · 2024-10-29T20:00:21Z

I agree this PR has a large footprint. However, I think the unit testing for states is really solid. In particular the regression testing has good coverage along with all the other unit tests of odd situations.

def test_states_2017(fast_sun_position_method):
    """
    Test for 200 days in 2017.  Includes 2017:066, 068, 090 anomalies and
    2017:250-254 SCS107 + 251 CTI.

I think that the new state classes that were added (which are not part of the "standard states" list) all had unit testing in the respective PR's. At least I assume that you would not have approved a new state class without a unit test?

taldcroft · 2024-10-29T20:22:48Z

And like you, I just ran the tests with coverage reporting. Overall the uncovered code seems unlikely to be impacted by this PR, but I guess that is the question to be answered.

taldcroft · 2024-10-29T20:48:56Z

Ugh, had to squash and rebase and fix a couple of unexpected merge conflicts. It seems OK now... 🤞

jeanconn · 2024-10-30T13:00:35Z

"At least I assume that you would not have approved a new state class without a unit test?" - there are no guarantees because of the evolution of code and us, but I think I always at least try to check to see "are the code changes introduced in this PR supported with unit tests, and if not, is there a good reason for that, and if that reason is valid are any such changes at least functionally tested". I think for this PR I also want to review the states diffs basically for the mission.

taldcroft · 2024-10-30T13:59:03Z

I think for this PR I also want to review the states diffs basically for the mission.

This may be time consuming, but sure. If you are doing this, then make sure to check every possible state, not just the "standard" states. You should be comparing the outputs to current master, not the release since there will be state change outputs in master vs release.

taldcroft · 2024-10-30T14:02:12Z

And of course as the reviewer you are free to ask me to do that validation.

And update some docs / comments

taldcroft · 2024-11-02T15:09:30Z

With the new notebook from 02ca6b2, the diffs are below. They are all expected as a result of the fix to not include transitions from a schedule NMAN maneuver after the safe mode transition.

diff --color -r validate/states-regression/master/dec.dat validate/states-regression/better-transitions/dec.dat
38242d38241
< 2023:044:17:42:12.432 50.72086847490219
38244d38242
< 2023:044:17:47:05.346 58.90670227829795
38246d38243
< 2023:044:17:51:58.259 55.07063366978747
38248d38244
< 2023:044:17:56:51.172 46.7699397703645
38250d38245
< 2023:044:18:01:44.085 43.8695729583467
diff --color -r validate/states-regression/master/off_nom_roll.dat validate/states-regression/better-transitions/off_nom_roll.dat
53641d53640
< 2023:044:17:42:12.432 -5.306268950562398
53643d53641
< 2023:044:17:47:05.346 -4.487472751051733
53645d53642
< 2023:044:17:51:58.259 -3.283765568468194
53647d53643
< 2023:044:17:56:51.172 -2.413068575051444
53649d53644
< 2023:044:18:01:44.085 -2.1760272604847017
53651,53654d53645
< 2023:044:18:25:30.816 -0.02932577056836294
< 2023:044:21:12:10.816 -0.13941266178335354
< 2023:044:23:58:50.816 -0.24949588227399033
< 2023:045:02:45:30.816 -0.35957552970671713
diff --color -r validate/states-regression/master/pcad_mode.dat validate/states-regression/better-transitions/pcad_mode.dat
13409d13408
< 2023:044:18:01:44.085 NPNT
diff --color -r validate/states-regression/master/pitch.dat validate/states-regression/better-transitions/pitch.dat
53641d53640
< 2023:044:17:42:12.432 117.98893502116009
53643d53641
< 2023:044:17:47:05.346 96.20165249364146
53645d53642
< 2023:044:17:51:58.259 77.91256597730393
53647d53643
< 2023:044:17:56:51.172 68.3272152655995
53649d53644
< 2023:044:18:01:44.085 66.17852995646959
53651,53654d53645
< 2023:044:18:25:30.816 90.01055688533111
< 2023:044:21:12:10.816 90.05018656860176
< 2023:044:23:58:50.816 90.08981465330565
< 2023:045:02:45:30.816 90.12944098712796
diff --color -r validate/states-regression/master/q1.dat validate/states-regression/better-transitions/q1.dat
38242d38241
< 2023:044:17:42:12.432 -0.16132465731723097
38244d38242
< 2023:044:17:47:05.346 -0.09407699034877226
38246d38243
< 2023:044:17:51:58.259 -0.02384061279114985
38248d38244
< 2023:044:17:56:51.172 0.025044611242553163
38250d38245
< 2023:044:18:01:44.085 0.0391529771
diff --color -r validate/states-regression/master/q2.dat validate/states-regression/better-transitions/q2.dat
38242d38241
< 2023:044:17:42:12.432 -0.7815727507319462
38244d38242
< 2023:044:17:47:05.346 -0.6918930404336519
38246d38243
< 2023:044:17:51:58.259 -0.577670760058282
38248d38244
< 2023:044:17:56:51.172 -0.4868528077736727
38250d38245
< 2023:044:18:01:44.085 -0.458937853
diff --color -r validate/states-regression/master/q3.dat validate/states-regression/better-transitions/q3.dat
38242d38241
< 2023:044:17:42:12.432 -0.4483065176694581
38244d38242
< 2023:044:17:47:05.346 -0.44861858677923044
38246d38243
< 2023:044:17:51:58.259 -0.4326475220719471
38248d38244
< 2023:044:17:56:51.172 -0.412558793639821
38250d38245
< 2023:044:18:01:44.085 -0.405409685
diff --color -r validate/states-regression/master/q4.dat validate/states-regression/better-transitions/q4.dat
38242d38241
< 2023:044:17:42:12.432 0.4026656887609058
38244d38242
< 2023:044:17:47:05.346 0.5578305338703091
38246d38243
< 2023:044:17:51:58.259 0.6917689211260268
38248d38244
< 2023:044:17:56:51.172 0.7695078643110088
38250d38245
< 2023:044:18:01:44.085 0.789611347
diff --color -r validate/states-regression/master/ra.dat validate/states-regression/better-transitions/ra.dat
38242d38241
< 2023:044:17:42:12.432 189.90121771639812
38244d38242
< 2023:044:17:47:05.346 225.8139182005564
38246d38243
< 2023:044:17:51:58.259 265.81594782693423
38248d38244
< 2023:044:17:56:51.172 285.7171146782722
38250d38245
< 2023:044:18:01:44.085 290.2936855229739
diff --color -r validate/states-regression/master/roll.dat validate/states-regression/better-transitions/roll.dat
38242d38241
< 2023:044:17:42:12.432 115.62109299947923
38244d38242
< 2023:044:17:47:05.346 87.24004950130909
38246d38243
< 2023:044:17:51:58.259 54.627072183787156
38248d38244
< 2023:044:17:56:51.172 39.99898342748677
38250d38245
< 2023:044:18:01:44.085 37.008652276993494

jeanconn · 2024-11-04T23:42:44Z

I did look further back, and one thing that may or may not be interesting is the NPNT transition for obsid 60399 in 2004. It looks like the maneuver to that ER was segmented. The new transitions infrastructure puts the NPNT transition 10 minutes later than the old transitions:

< 2004:023:04:41:59.949 NPNT
---
> 2004:023:04:51:28.403 NPNT

And from kadi events it looks like the old answer was better

In [16]: from  kadi import events

In [17]: ms = events.manvrs.filter(obsid=60399)

In [18]: ms
Out[18]: 
<Manvr: start=2004:023:03:53:22.647 dur=1048 n_dwell=0 template=interm_att>
<Manvr: start=2004:023:04:11:10.697 dur=1849 n_dwell=1 template=normal>

In [19]: ms[1].manvr_stop
Out[19]: '2004:023:04:42:00.310'

I just looked back until 2004:001 and was a bit surprised by this change.

taldcroft · 2024-11-05T11:42:01Z

I did look further back, and one thing that may or may not be interesting is the NPNT transition for obsid 60399 in 2004.

This is the result of commands from this maneuver-only load not being included in the 2004 commands:
https://icxc.harvard.edu//mp/mplogs/2004/JAN2104/oflsa/starcheck.html

Basically that missing maneuver sets off a chain of issues because kadi is predicting the wrong maneuver time. Commanded states (both in master and with this PR) are just wrong, but e.g. in master you find the pitch changing rapidly while in NPNT.

With the new code things are different for the subsequent segmented manuever you highlighted because it refuses to apply attitude updates in NPNT, so those maneuvering-in-NPNT commands are dropped.

Anyway, after 2.5 hours of investigation I can confirm that this is not interesting.

jeanconn

Looks good to me - with solid testing. And thanks for looking into the one historical oddity I ran into that turned out to be a cmds error and not a regression.

taldcroft changed the base branch from master to matlab-support-nsm-ssm-offsets October 27, 2024 11:04

taldcroft requested review from javierggt and jeanconn October 28, 2024 12:36

taldcroft changed the title ~~WIP: improve transitions~~ Improve command states with better transition infrastructure Oct 28, 2024

taldcroft marked this pull request as ready for review October 28, 2024 12:37

This was referenced Oct 28, 2024

Support offset pitch for Safe mode and NSM events and improve Safe mode #334

Merged

Set min_violation_duration to 180s for pcad validation #338

Merged

Base automatically changed from matlab-support-nsm-ssm-offsets to master October 29, 2024 16:44

jeanconn reviewed Oct 29, 2024

View reviewed changes

kadi/commands/states.py Outdated Show resolved Hide resolved

jeanconn reviewed Oct 29, 2024

View reviewed changes

kadi/commands/states.py Outdated Show resolved Hide resolved

taldcroft force-pushed the better-transitions branch from bfea926 to 1a6f5b5 Compare October 29, 2024 20:45

taldcroft force-pushed the better-transitions branch from 45ba7a1 to 29aa3d7 Compare November 2, 2024 14:46

taldcroft added 3 commits November 2, 2024 10:49

Improved transitions

ff5bdb1

Add default_value for HRC transitions

1ed3352

Change Transition constraint to callable function

c40f3b4

And update some docs / comments

taldcroft force-pushed the better-transitions branch from 29aa3d7 to c40f3b4 Compare November 2, 2024 14:49

Add notebook for comprehensive ~5 year states regression test

02ca6b2

taldcroft requested a review from jeanconn November 2, 2024 15:09

jeanconn approved these changes Nov 5, 2024

View reviewed changes

taldcroft merged commit 56e60d8 into master Nov 5, 2024
4 checks passed

taldcroft deleted the better-transitions branch November 5, 2024 16:40

javierggt mentioned this pull request Dec 9, 2024

2024.12 sot/skare3#1450

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve command states with better transition infrastructure #340

Improve command states with better transition infrastructure #340

taldcroft commented Oct 27, 2024 •

edited by jeanconn

Loading

jeanconn commented Oct 29, 2024

jeanconn commented Oct 29, 2024

taldcroft commented Oct 29, 2024

taldcroft commented Oct 29, 2024

taldcroft commented Oct 29, 2024

jeanconn commented Oct 30, 2024

taldcroft commented Oct 30, 2024

taldcroft commented Oct 30, 2024

taldcroft commented Nov 2, 2024

jeanconn commented Nov 4, 2024

taldcroft commented Nov 5, 2024 •

edited

Loading

jeanconn left a comment

Improve command states with better transition infrastructure #340

Improve command states with better transition infrastructure #340

Conversation

taldcroft commented Oct 27, 2024 • edited by jeanconn Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

jeanconn commented Oct 29, 2024

jeanconn commented Oct 29, 2024

taldcroft commented Oct 29, 2024

taldcroft commented Oct 29, 2024

taldcroft commented Oct 29, 2024

jeanconn commented Oct 30, 2024

taldcroft commented Oct 30, 2024

taldcroft commented Oct 30, 2024

taldcroft commented Nov 2, 2024

jeanconn commented Nov 4, 2024

taldcroft commented Nov 5, 2024 • edited Loading

jeanconn left a comment

Choose a reason for hiding this comment

taldcroft commented Oct 27, 2024 •

edited by jeanconn

Loading

taldcroft commented Nov 5, 2024 •

edited

Loading