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 issues with thermal model handling #9

Merged
merged 10 commits into from
Dec 1, 2021

Conversation

jzuhone
Copy link
Member

@jzuhone jzuhone commented Nov 10, 2021

This PR fixes a number of issues related to ACISpy's handling for thermal models.

  • The call to get_xija_model_spec in the function find_json to get the model specification file originally set check_version=True, but this function was often called many times in a short interval in some applications and github did not appreciate the repeated requests. So this option is now turned off.
  • The find_json function also had some convoluted logic to look for the JSON file in a different chandra_models repository path which has now been fixed.
  • The ThermalModelFromRun class "divided" a string by a string instead of using the divide operator to concatenate a Path object and a string. This has now been fixed.
  • The ThermalModelRunner.from_backstop had an error in computing the initial temperature from the start of a model run. This has now been fixed, and required an update to one of the test answers.
  • A new pytest fixture has been added to allow one to store new answers when running the tests if necessary.
  • The interface with acis_thermal_check has been updated to match the changes made in Move all of the temperature model scripts into the main package acis_thermal_check#44.
  • The planning limit for 1DEAMZT model was one degree lower than it should have been and this is fixed.

All regression tests pass with the answers fixed, and the new answers were hand-checked against the old. The new functionality to store answers has also been checked to ensure that the answers are identical if they are supposed to be the same.

@jzuhone
Copy link
Member Author

jzuhone commented Nov 10, 2021

@taldcroft does this need to go through the new control board process? it's not used for load review but it does get installed into skare.

@jzuhone
Copy link
Member Author

jzuhone commented Nov 30, 2021

@taldcroft just wanted to ask again if you thought this needed to go through FSDS (my supposition is not, but if you want to update it in skare you might have to?)

@taldcroft
Copy link

Sorry I missed the first comment @ mention. No, this does not need to go as a separate FSDS issue, but the assumption is that each PR that gets lumped into a Ska update (which goes to FSDS) has been reviewed internally, passes some sort of testing and does not break other stuff. We apply that to every Ska package update.

@jzuhone
Copy link
Member Author

jzuhone commented Nov 30, 2021

Ok--it's not necessary for this to go into the same ska update as acis_thermal_check if it's not convenient. It can wait until the next one.

@jzuhone jzuhone merged commit 4fdceb1 into acisops:master Dec 1, 2021
@jeanconn jeanconn mentioned this pull request Dec 3, 2021
@javierggt javierggt mentioned this pull request Aug 3, 2022
@jzuhone jzuhone deleted the thermal_model_fixes branch February 20, 2023 17:39
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