Make maude_decom.get_aca_images robust agains missing/repeated VCDU frames #159

javierggt · 2023-10-24T18:41:58Z

Description

This PR fixes two issues that were found while reading monitoring data:

maude_decom.get_aca_images crashes if stop is after the last frame available in MAUDE. In this case some maude queries return no data, and the downstream code should not crash.
maude_decom.get_aca_images crashes if there are missing/repeated frames.

A test was added for the case of repeated/missing frames.

Interface impacts

Testing

Unit tests

No unit tests
Mac
Linux
Windows

Independent check of unit tests by Jean

Mac

Functional tests

No functional testing.

taldcroft · 2023-11-11T11:31:13Z

chandra_aca/maude_decom.py

+        - VCDU counters come in packets of four, with each group starting with a multiple of 4.
+    """
+    current_packet = []  # to group in fours
+    last_frame = 0  # for monotonicity check


last_frame is never updated so the check below last_frame > vcdu_counter is not doing anything.

I added a fix for this and included a corresponding test case.

taldcroft · 2023-11-11T11:41:13Z

chandra_aca/maude_decom.py

@@ -601,6 +601,29 @@ def _group_packets(packets, discard=True):
        yield res


+def filter_vcdu_jumps(vcdu_counters):
+    """
+    Return a boolean mas to filter VCDU counters that are not continuous.


There is the annoying complication of the VCDU counter rolling over. While it is rare, this function should handle that. I think the rollover is 2**18 but that needs to be checked. Basically if you see a negative delta (from one VCDU to the next) of more than (2**18 - 3600 * 4) then all the counters after that point should be incremented by 2**18.

The 3600*4 is meant to reflect the largest number of VCDU's that could be dropped during a real-time support. You need to protect against a rollover happening right in the middle of a loss of comm during telemetry which would otherwise mask out every VCDU frame after the loss of comm.

I added a fix for this and included a corresponding test case.

The VCDU frame rolls over at 2^24. I used the maximum number of missing frames as 3600*4 as you said here, but why is this the right number?

For the VCDU rollover, I see there a new test with what I would call "synthetic" VCDUs... from this work can you point me to a handy real rollover or should I go find one?

The 3600 * 4 is a bit arbitrary. That corresponds to a one-hour comm loss. In the end it doesn't matter all that much since the worst case scenario (which is incredibly unlikely) is dropping one image.

@jeanconn

ska_tdb.msids.find("vcdu", "counter") dat = fetch.Msid("CVCDUCTR", "2023:001", "2023:120", stat="5min") %matplotlib dat.plot()

This is a bit interesting because it shows some resets in the safe modes.

Thanks! I don't know how far we need to get to the solution in this PR, but it looks like your changes helped the aca_view test a bit, in that it looks like the aca_view playback was no longer stuck at 2023:296:22:20:35 . But in playback it also didn't show any new ACA data changes for the rest of the interval (time and att updating but all the ACA data stuck).

This is not the behavior I see. I ran this:

python -m aca_view.scripts.aca_viewer --start 2023:296:22:20:26 --stop 2023:296:22:20:50

and I see the data updating after the rollover

For me, I was running

python -m aca_view.scripts.aca_viewer --start 2023:296:22:20:00.000 --stop 2023:296:22:24:00.000 --maude

And it looks like it stops working at about 2023:296:22:20:40.301

ok, I can confirm the issue in aca_view. One can check it by starting aca_view, opening the ipython widget, and typing:

from cxotime import CxoTime CxoTime(timeline.table['TIME'][-1]).date Out[3]: '2023:296:22:21:59.837' CxoTime(timeline.slots[0].table['TIME'][-1]).date Out[4]: '2023:296:22:20:39.887'

I think this is an aca_view issue, because running chandra_aca.maude_decom.get_aca_images returns correct data:

In [1]: from chandra_aca import maude_decom ...: from cxotime import CxoTime ...: start = '2023:296:22:20:26' ...: stop = '2023:296:22:22:00' ...: images = maude_decom.get_aca_images(start, stop) ...: print(CxoTime(images[images['IMGNUM'] == 0]['TIME'][-1]).date) ...: np.asarray(images[images['IMGNUM'] == 0]['IMG'][-1]) 2023:296:22:21:55.914 Out[1]: array([[ 31. , 112. , 91.75, 71.5 , 139. , 98.5 , 44.5 , 17.5 ], [ 132.25, 44.5 , 105.25, 267.25, 442.75, 226.75, 64.75, 31. ], [ 71.5 , 105.25, 267.25, 706. , 753.25, 469.75, 166. , 132.25], [ 91.75, 321.25, 1286.5 , 5788.75, 5289.25, 1192. , 280.75, 226.75], [ 71.5 , 280.75, 1246. , 6855.25, 6841.75, 1711.75, 361.75, 98.5 ], [ 24.25, 78.25, 287.5 , 1192. , 1151.5 , 510.25, 247. , 64.75], [ 17.5 , 64.75, 152.5 , 152.5 , 247. , 233.5 , 145.75, 71.5 ], [ 24.25, 24.25, 51.25, 71.5 , 118.75, 37.75, 64.75, 44.5 ]])

so I think the integration issue is not part of this PR.

taldcroft · 2023-11-13T16:16:39Z

chandra_aca/tests/test_maude_decom.py

+    )
+    assert len(packets) == 32
+
+    # the following checks the underlying function used to filter out repeated/missing frames


Can you write these tests like below. This will make it much easier for me to review the tests.

filter_vcdu_test_cases = [ { "inp": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9], "exp": [1, 1, 1, 1, 1, 1, 1, 1, 0, 0], }, { "inp": [0, 1, 3, 2, 4, 5, 6, 7, 8, 9], "exp": [0, 0, 0, 0, 1, 1, 1, 1, 0, 0], }, { "inp": [0, 1, 2, 4, 5, 6, 7, 8, 9], "exp": [0, 0, 0, 1, 1, 1, 1, 0, 0], }, ..., ] @pytest.mark.parametrize("case", filter_vcdu_test_cases) def test_filter_vcdu_jumps(case): ...

You did something different than I asked. Your version does not demonstrate how the code behaves for repeated VCDU's.

{ "inp": np.array([1, 2, 3, 4, 5, 6, 6, 7, 8, 9]), "exp": np.array([4, 5, 6, 7]), # Which vcdu=6 was selected? },

Versus

{ "inp": [1, 2, 3, 4, 5, 6, 6, 7, 8, 9], "exp": [0, 0, 0, 1, 1, 1, 0, 1, 0, 0], },

ok, I just changed that.

I give up. What you did still looks nothing like what I asked for.

I'm sorry there is some miscommunication. Perhaps you could invest just a couple of minutes (I doubt it would take you more than two minutes) to convince yourself that this checks what you requested.

The only difference is that what you wrote above gives an integer index for the resulting groups, whereas I gave a boolean mask that tells you which frame was selected.

For example, in this case:

"inp": np.array([1, 2, 3, 4, 5, 6, 6, 7, 8, 9]), "exp": np.array( [False, False, False, True, True, True, False, True, False, False] ), },

frame 6 is repeated, and the one selected is the first one.

and by the way: I understood your request above as '1' and '0' being boolean, and you wanted to check which instance of a repeated frame was chosen, so what is in the test is equivalent to (with bool arrays as int)

"inp": np.array([1, 2, 3, 4, 5, 6, 6, 7, 8, 9]), "exp": np.array( [0, 0, 0, 1, 1, 1, 0, 1, 0, 0] ),

which is verbatim what you wrote above.

I guess I didn't explain. The entire point of what I wrote was so that the mask values would visually line up with the vcdu numbers. That would make it easy for me to review and make sure the output is what I would expect.

The way you wrote it is just too much work for me as a reviewer to mentally line things up, so I didn't bother and just 🤞 it's OK.

Also the np.array wrappers are not in what I wrote and they can cause the numbers to not line up visually again.

yes, I totally did not get that what you meant was to line up visually. I understood that what you meant was to actually check which frame was being selected (the previous version of the test did not check that, and that is the change I made).

You also did not pull the test cases out and use pytest.mark.parametrize as I requested. I think you understand why that is important for testing.

jeanconn · 2023-11-14T17:53:03Z

It looks like @taldcroft's comments are substantive, so feel free to ping me for a new review when those issues are resolved.

…e of missing/repeated frames in telemetry

…d test case) (last_frame was not being used)

…test case)

…inal result

jeanconn

I'm good with these changes and their testing.

taldcroft · 2023-12-19T16:10:27Z

I'm delegating to @jeanconn's review and bowing out.

javierggt force-pushed the ir-monitor branch from 00527ad to 2a95740 Compare November 9, 2023 18:18

javierggt changed the title ~~WIP: Fix errors found when reading monitor windows in IR zone~~ Make maude_decom.get_aca_images robust agains missing/repeated frames Nov 9, 2023

javierggt requested review from taldcroft and jeanconn November 9, 2023 18:22

taldcroft reviewed Nov 13, 2023

View reviewed changes

jeanconn removed their request for review November 14, 2023 17:53

javierggt added 6 commits December 11, 2023 09:14

return empty data if there is no data

0ac13c6

add maude_decom failing test for cases with repeated frames in telemetry

feef02c

Fix in maude_decom.get_raw_aca_packets: make it more resilient in cas…

411d17b

…e of missing/repeated frames in telemetry

wrote test_maude_decom.py::test_filter_vcdu_jumps in a clearer way

9fee459

fix maude_decom.filter_vcdu_jumps to handle repeated frames (and adde…

c9cd3d1

…d test case) (last_frame was not being used)

fix maude_decom.filter_vcdu_jumps to handle VCDU rollover (and added …

bcd2671

…test case)

javierggt force-pushed the ir-monitor branch from 2a95740 to bcd2671 Compare December 11, 2023 17:35

javierggt requested review from taldcroft and jeanconn December 11, 2023 17:37

javierggt added 3 commits December 12, 2023 18:11

fix in maude_decom._group_packets: handle VCDU rollover

ab1e9f1

docstrings

fa642a1

test_maude_decom::test_filter_vcdu_jumps: test on the mask, not the f…

20780b5

…inal result

jeanconn approved these changes Dec 13, 2023

View reviewed changes

taldcroft removed their request for review December 19, 2023 16:10

javierggt merged commit bf34134 into master Dec 19, 2023
2 checks passed

This was referenced Jan 9, 2024

Update sot/chandra_aca to 4.45.0 sot/skare3#1224

Closed

2023.10 sot/skare3#1242

Merged

javierggt mentioned this pull request Feb 6, 2024

2023.11 sot/skare3#1246

Merged

javierggt deleted the ir-monitor branch August 29, 2024 16:51

javierggt changed the title ~~Make maude_decom.get_aca_images robust agains missing/repeated frames~~ Make maude_decom.get_aca_images robust agains missing/repeated VCDU frames Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make maude_decom.get_aca_images robust agains missing/repeated VCDU frames #159

Make maude_decom.get_aca_images robust agains missing/repeated VCDU frames #159

javierggt commented Oct 24, 2023 •

edited by jeanconn

Loading

taldcroft Nov 11, 2023

javierggt Dec 11, 2023

taldcroft Nov 11, 2023

javierggt Dec 11, 2023

jeanconn Dec 11, 2023

taldcroft Dec 11, 2023

taldcroft Dec 11, 2023

jeanconn Dec 13, 2023

javierggt Dec 13, 2023

jeanconn Dec 13, 2023

javierggt Dec 13, 2023

javierggt Dec 13, 2023

taldcroft Nov 13, 2023 •

edited

Loading

javierggt Dec 11, 2023

taldcroft Dec 13, 2023

javierggt Dec 13, 2023

taldcroft Dec 19, 2023

javierggt Dec 19, 2023

javierggt Dec 19, 2023 •

edited

Loading

taldcroft Dec 19, 2023 •

edited

Loading

javierggt Dec 19, 2023

taldcroft Dec 19, 2023

jeanconn commented Nov 14, 2023

jeanconn left a comment

taldcroft commented Dec 19, 2023

Make maude_decom.get_aca_images robust agains missing/repeated VCDU frames #159

Make maude_decom.get_aca_images robust agains missing/repeated VCDU frames #159

Conversation

javierggt commented Oct 24, 2023 • edited by jeanconn Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taldcroft Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierggt Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

taldcroft Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn commented Nov 14, 2023

jeanconn left a comment

Choose a reason for hiding this comment

taldcroft commented Dec 19, 2023

javierggt commented Oct 24, 2023 •

edited by jeanconn

Loading

taldcroft Nov 13, 2023 •

edited

Loading

javierggt Dec 19, 2023 •

edited

Loading

taldcroft Dec 19, 2023 •

edited

Loading