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

Factor out get_ofp_states and get_telem_values for cheta #249 improvements #285

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Apr 8, 2023

Description

This PR is paired with sot/cheta#249 to factor out the get_ofp_states and get_telem_values functions into cheta where they more logically belong.

Interface impacts

get_ofp_states and get_telem_values are no longer available in kadi.utils. Both of these are currently used only internally within cheta and kadi in SOT repos on GitHub. Any other usages in non-versioned tools can just be fixed.

The API for both of these were changed as well, hence it would not make sense to include a reference in kadi.utils (from cheta) for back-compatibility.

Testing

Unit tests

  • Mac

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

Validation script was run from inside the repo for the branch outputs and outside the repo for release. The cheta repository was checked out with sot/cheta#249.

In both cases below I did a side-by-side visual / by-eye text comparison of the outputs from this branch and ska3 2023.1 respectively. No diffs were seen.

Run with default arguments covering last 14 days (uses MAUDE)

env PYTHONPATH=/Users/aldcroft/git/cheta python -m kadi.scripts.validate_states --out-dir current-branch
python -m kadi.scripts.validate_states --out-dir current-release

Run with arguments covering 2022:283 safe mode (no MAUDE)

env PYTHONPATH=/Users/aldcroft/git/cheta python -m kadi.scripts.validate_states --out-dir safe-2022-branch --stop=2022:297 --days=5
python -m kadi.scripts.validate_states --out-dir safe-2022-release --stop 2022:297 --days=5

@taldcroft taldcroft changed the title WIP: Refactor for cheta improvements Factor out get_ofp_states and get_telem_values for cheta #249 improvements Apr 9, 2023
@@ -135,9 +136,16 @@ def __init_subclass__(cls, **kwargs):
@property
def tlm(self):
if not hasattr(self, "_tlm"):
self._tlm = get_telem_values(
msids=self.msids, stop=self.stop, days=self.days
logger.info(
Copy link
Member Author

Choose a reason for hiding this comment

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

The logging and exception handling was previously inside get_telem_values.

@@ -62,6 +62,7 @@ def main(args=None):
# Enable logging in relevant packages
logging.getLogger("kadi").setLevel(opt.log_level)
fetch.add_logging_handler(level=opt.log_level)
fetch.data_source.set("cxc", "maude allow_subset=False")
Copy link
Member Author

Choose a reason for hiding this comment

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

Using MAUDE was previously hard-coded into get_telem_values which limits its general-purpose utility. Instead we set the data source globally within the validate states app.

Copy link
Contributor

@javierggt javierggt left a comment

Choose a reason for hiding this comment

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

The code changes seem simple enough to me:

  • functions got removed
  • replacements from cheta are used
  • logging and error check are added
  • some other tiny things

I looked over the functional test reports and ran the scripts on the branch. It seems ok to me. I just want to note that I got a slightly different TSCPOS plot. They agree between master and branch, but differ with yours. I put all the reports here. I do not think it is relevant to this PR, but still begs the question.

I suggest to either:

@taldcroft
Copy link
Member Author

@javierggt - thanks for the review. I agree we need to wait on cheta# 249. About the diffs, for the record here it is and I'll look into it. I also note that plotly seems to be doing something weird with the negative sign. This occurs in all the versions so it is not a regression here but still should be fixed.

TA

image

JG

image

@taldcroft
Copy link
Member Author

@javierggt - about the difference in SIM-Z between our respective runs, I note that the additional bad data point in your plot is within a couple seconds of a time when the OFP is doing something weird at 2022:294:16:37:27.347 CONLOFP=STUP.

When I directly look at telemetry from cheta this glitch does not seem present:

In [13]: from cheta import fetch_eng

In [14]: dat = fetch_eng.Msid("3tscpos", "2022-10-21", "2022-10-22")

In [15]: set(dat.vals)
Out[15]: {-99616.0}

Do you get the same?

@taldcroft
Copy link
Member Author

About the malformed minus sign, this is discussed at https://stackoverflow.com/questions/75234217/gibberish-malformed-negative-y-axis-values-in-plotly-charts-in-python, but with no resolution.

@taldcroft
Copy link
Member Author

cheta 249 is merged now, so I'm merging this one.

@taldcroft taldcroft merged commit 563b759 into master Apr 24, 2023
@taldcroft taldcroft deleted the fix-get-ofp-states branch April 24, 2023 17:14
This was referenced Jun 1, 2023
@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.

2 participants