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

Fix problems that came up in pitch/roll-comp when using real-time data #250

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Apr 30, 2023

Description

Using MAUDE data just after a comm for the pitch_comp and roll_comp computed MSIDs was failing for a couple of reasons:

  • There was a 1-sample (2.05 sec) CONLOFP state that caused a problem in state_intervals. This was fixed by specifying the start and stop times (enabled by Improve pitch/roll_comp, logical_intervals, state_intervals, get_ofp_states, get_telem_table #249).
  • The chunk of time after the comm where ephemeris data are available but no quaternions was being marked as bad by the telemetry fetching code but that was being used in subsequent processing to just set the pitch or roll to 0.0. Instead the data points need to be removed entirely. The updated code does this now.

Interface impacts

None

Testing

Unit tests

  • Mac

Independent check of unit tests by Jean

  • Linux

Functional tests

Using this branch I ran a fetch query for pitch_comp about one hour after BOT. The query started before the comm and ended at the current time (after the comm). Before the fix this either failed completely (without the CONLOFP fix) or returned a bunch of 0.0 values at the end. After the fix I saw the expected pitch values.

@taldcroft taldcroft requested review from javierggt and jeanconn June 28, 2023 18:27
@jeanconn
Copy link
Contributor

Does this need a unit test?

@javierggt
Copy link
Contributor

If no unit test is required, it would be nice to specify the functional test done (meaning the exact code run and the expected output in flight and this branch).

Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

I'm going to change my review to "this probably deserves a unit test or updated unit test unless there's a good explanation about why that's too hard or not possible"

@taldcroft
Copy link
Member Author

Here is a demonstration of one type of failure that is seen in flight ska 2023.1, done at around 14:50z on 2023:190. Unfortunately I'm still working on a reproducible demo, since this one stopped happening later on (presumably related to the MAUDE archive filling in).

In [19]: os.environ[
    ...:     "CHETA_FETCH_DATA_GAP"
    ...: ] = f"--exclude=*EPHEM* --start={(now - 1*u.hr).date} --stop={now.date}"

In [20]: os.environ["CHETA_FETCH_DATA_GAP"]
Out[20]: '--exclude=*EPHEM* --start=2023:190:13:35:46.305 --stop=2023:190:14:35:46.305'

In [21]: dat = fetch.Msid("pitch_comp", now - 2 * u.hr, now)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[21], line 1
----> 1 dat = fetch.Msid("pitch_comp", now - 2 * u.hr, now)

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/fetch.py:1844, in Msid.__init__(self, msid, start, stop, filter_bad, stat)
   1843 def __init__(self, msid, start=LAUNCH_DATE, stop=None, filter_bad=True, stat=None):
-> 1844     super(Msid, self).__init__(
   1845         msid, start=start, stop=stop, filter_bad=filter_bad, stat=stat
   1846     )

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/fetch.py:572, in MSID.__init__(self, msid, start, stop, filter_bad, stat)
    570 # Get the times, values, bad values mask from the HDF5 files archive
    571 if intervals is None:
--> 572     self._get_data()
    573 else:
    574     self._get_data_over_intervals(intervals)

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/fetch.py:637, in MSID._get_data(self)
    635 comp_cls = ComputedMsid.get_matching_comp_cls(self.msid)
    636 if comp_cls:
--> 637     self._get_comp_data(comp_cls)
    638     return
    640 # Avoid stomping on caller's filetype 'ft' values with _cache_ft()

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/fetch.py:703, in MSID._get_comp_data(self, comp_cls)
    700 logger.info(f"Getting computed values for {self.msid}")
    702 # Do computation.  This returns a dict of MSID attribute values.
--> 703 attrs = comp_cls(self.units["system"])(
    704     self.tstart, self.tstop, self.msid, self.stat
    705 )
    707 # Allow upstream class to be a bit sloppy on times and include samples
    708 # outside the time range.  This can happen with classes that inherit
    709 # from DerivedParameter.
    710 ok = (attrs["times"] >= self.tstart) & (attrs["times"] <= self.tstop)

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/derived/comps.py:250, in ComputedMsid.__call__(self, tstart, tstop, msid, interval)
    244 match_args = [
    245     arg.lower() if isinstance(arg, str) else arg for arg in match.groups()
    246 ]
    248 if interval is None:
    249     # Call the actual user-supplied work method to compute the MSID values
--> 250     msid_attrs = self.get_msid_attrs(tstart, tstop, msid.lower(), match_args)
    252     for attr in ("vals", "bads", "times", "unit"):
    253         if attr not in msid_attrs:

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/derived/comps.py:692, in Comp_Pitch_Roll_OBC_Safe.get_msid_attrs(self, tstart, tstop, msid, msid_args)
    690 # Get states of either NPNT / NMAN or NSUN
    691 vals = np.isin(dat.vals, ["NPNT", "NMAN"])
--> 692 states_npnt_nman = logical_intervals(
    693     dat.times, vals, complete_intervals=False, max_gap=2.1
    694 )
    695 states_npnt_nman["val"] = np.repeat("NPNT_NMAN", len(states_npnt_nman))
    696 # Require at least 10 minutes => 2 samples of the ephem data. This is
    697 # needed for the built-in derived parameter calculation to work.

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/utils.py:289, in logical_intervals(times, bools, complete_intervals, max_gap)
    286 if max_gap is not None:
    287     times, bools = _pad_long_gaps(times, bools, max_gap)
--> 289 intervals = state_intervals(times, bools)
    291 if complete_intervals:
    292     if len(intervals) > 0 and intervals["val"][0]:

File ~/miniconda3/envs/ska3/lib/python3.10/site-packages/cheta/utils.py:334, in state_intervals(times, vals)
    331 from astropy.table import Table
    333 if len(vals) < 2:
--> 334     raise ValueError("Filtered data length must be at least 2")
    336 transitions = np.hstack([[True], vals[:-1] != vals[1:], [True]])
    337 t0 = times[0] - (times[1] - times[0]) / 2

ValueError: Filtered data length must be at least 2

@taldcroft
Copy link
Member Author

I did roughly the same thing using this branch a few minutes later and saw no problem. But I can't absolutely confirm that this PR fixed things since flight ska started working OK at some point as well.

@jeanconn
Copy link
Contributor

jeanconn commented Jul 9, 2023

For the missing-data fix, is the hypothesis now that this happens if the MAUDE data for some of the quaternions isn't available (either missing 1 or more of 4 or have some new data but not some older data?). It seems like those cases could be constructed to show the new code works better, but it isn't clear to me what MAUDE would be doing.

@taldcroft taldcroft merged commit 9fb8d25 into master Sep 13, 2023
@taldcroft taldcroft deleted the more-pitch-roll-comp-fixes branch September 13, 2023 17:42
This was referenced Sep 18, 2023
@javierggt javierggt mentioned this pull request Oct 4, 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.

3 participants