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

Use chandra limits #71

Merged
merged 44 commits into from
Apr 12, 2024
Merged

Use chandra limits #71

merged 44 commits into from
Apr 12, 2024

Conversation

jzuhone
Copy link
Member

@jzuhone jzuhone commented Mar 11, 2024

Description

This version of acis_thermal_check hands violations checking and plotting of limits over completely to the chandra_limits package. A review of the most important changes is as follows:

  • As noted above, the violations checking code has been ripped out. The code that is now in chandra_limits is largely copied and refactored over from the code originally in acis_thermal_check.
  • For the ACIS FP model, instead of reporting a table of "hot ACIS" observations, there is a new page linked to from the main page reporting a table of all observations and their properties in the schedule.
  • The planning limit lines are now piece-wise constant, especially in the the case of the ACIS FP. Yellow limit lines remain the same.
  • In the ACIS FP plots, the obsids are now only colored red and green depending on whether they are ACIS-I or ACIS-S.
  • The validation limits code has been simplified as there was a lot of duplication. This largely occurred in commit d591e91.

Practical effects of these changes:

  • Plots look somewhat different because of the change in planning limits, especially the ACIS FP plot.
  • ACIS FP data quality violations are no longer reported during the taking of a bias.
  • ACIS FP -105 C observations are now supported. One of the plots has had its upper limit set to -103 C to accommodate this.

Interface impacts

The table reporting violations on the webpages has changed slightly, but is human-read. Plots show limit lines as a function of time instead of every limit line across the whole plot; also human-read.

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

All tests which do not depend on violations checking pass without any answer updates as expected. Some answer changes are necessary for the FP and a couple of other models. All of these answer changes were hand-checked and they were found to be correct based on the code changes.

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

Ran the code in this PR at weekly load reviews for several weeks and results were checked for consistency with flight code; any differences are explained by changes to the violations checking changes.

jzuhone and others added 30 commits March 6, 2024 15:09
…L file. Also print the limit that is violated in the table
@jzuhone jzuhone marked this pull request as ready for review March 26, 2024 14:15
Copy link

Choose a reason for hiding this comment

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

Just want to confirm the intentionality of having syntax of odb.caution for the limits for the official models vs the safety.caution for the "informal" models.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not so much "official" vs "informal" as what limits are stored in the ODB (related to engineering telemetry) vs the ones related to DEA HKP telemetry

# For the ACIS FP limit we write out an obsid table
context["acis_obs"] = self.acis_and_ecs_obs
template_path = (
TASK_DATA / "acis_thermal_check/templates/obsid_template.rst"
Copy link

Choose a reason for hiding this comment

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

I don't see where TASK_DATA is defined.

Choose a reason for hiding this comment

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

In passing - I think it is imported from utils here

@jazan12
Copy link

jazan12 commented Mar 28, 2024 via email

@jazan12
Copy link

jazan12 commented Mar 28, 2024 via email

Copy link

@jazan12 jazan12 left a comment

Choose a reason for hiding this comment

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

All my comments were addressed. Looks good!

@Gregg140 Gregg140 self-requested a review April 12, 2024 15:51
@Gregg140
Copy link
Contributor

Looks good to me.

@jzuhone jzuhone merged commit 684e767 into master Apr 12, 2024
1 check passed
@javierggt javierggt mentioned this pull request May 1, 2024
@javierggt javierggt mentioned this pull request Jun 26, 2024
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.

4 participants