-
Notifications
You must be signed in to change notification settings - Fork 3
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
Validate kadi states #261
Validate kadi states #261
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I went over the code and it seems fine. There are only two minor comments.
- I successfully ran unit tests.
- I ran the validate script, but the
maude.config
submodule is not there, so I had to manually comment it out (the file seems to be missing in maude's PR #44).
I noticed that the setup.py
still lists all the templates, so I just want to point out that those templates were removed. Currently, the only templates in kadi are the ones added by this PR.
If this is implemented in the kadi web server, most likely the templates will not be used, because we will want to use the kadi style. I suppose it is ok to leave them here, because this is currently a standalone application.
kadi/commands/utils.py
Outdated
:param msids: fetch msids list | ||
:param stop: stop time for telemetry (CxoTime-like) | ||
:param days: length of telemetry request before ``tstart`` | ||
:param name_map: dict mapping msid to recarray col name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this argument does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
This is possible with plotly figures:
so in principle one can always recreate the figure from the json. That means the web app has the choice of rendering the html in the server or sending the json and rendering in the browser. One option for a web page could be to request each validation plot separately in json and load them as they are done, but it might not be a good idea because there might be repeated calls to maude spread over several python instances. |
Fixed. |
Yes, we can cross that bridge later with the app. Likewise the interesting idea about returning the validation plot as JSON in order to have a more responsive app. |
8e9ad6b
to
8f9642f
Compare
Description
This is the modern Py3 version of
validate_states
. Instead of updating that project and making a new Ska3 package, this PR makes a new sub-package of kadi (with a corresponding app) to handle state validation.This requires sot/cheta#242.
Known issues / To do
In-work
command eventsOptional
Interface impacts
This will supersede the Py2
validate_states
app.Testing
Unit tests
Independent check of unit tests by Jean. I ran with sot/cheta#242, https://github.com/sot/maude/pull/44, sot/cxotime#31 in my path.
Functional tests
Run over full 2022:293 IU-reset and Safe mode
This gives the expected results.
Run within 2022:293 IU-reset and Safe mode (at day 295:0000z)
This creates a data gap corresponding to being in a real-time comm after an 8-hour gap.
This gives the expected results.