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

Adding model specifications for BEP and FEP models #62

Merged
merged 7 commits into from
Jul 20, 2021

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented Apr 10, 2020

This PR adds the model specifications for these BEP and FEP models which ACIS Ops runs during load reviews, but are not yet used for flight approval. These correspond to the FEP1 Actel, FEP1 Mongoose, and BEP PCB temperatures in the ACIS DPA. The check tools for these models will eventually be installed in ska3-flight, see sot/skare3#323 and sot/skare3#325.

I also added a .gitignore to ignore files which appear when this is installed as a Python package.

These models are run at every load review by the ACIS ops team and we examine the validation plots at our weekly tagup. They are not used in the actual review itself, but to monitor the predictions for these temperatures and the agreement of the models with the data. These models are definitely WIP in that some features are not modeled well currently (such as low excursions of the FEP temperatures).

The yellow/caution limits are taken from here:

https://cxc.cfa.harvard.edu/acis/PMON/pmon_limits.txt

and we assume a 2 degree backoff for the "planning" limit in each case, consistent with model accuracy.

@taldcroft
Copy link
Member

One thing I wonder is if we should revisit the current strategy of bundling the model spec with every checker tool instead of using chandra_models as the central repository for xija models. I'm not adamant either way but somehow it seems cleaner to have a single definitive source for all of the models. Barring structural changes, there is no real reason the checker code should need to be re-released for a simple model update (like the new way of MATLAB tools). I guess the flip side is that for structural changes to models it is somewhat easier to manage running and testing the tool at different versions since the spec file is bundled. Anyway just thinking out loud.

@jzuhone
Copy link
Contributor Author

jzuhone commented Jul 1, 2021

I'd like to pick this back up a year later--since the next version of acis_thermal_check will be using the model specification files from chandra_models, I'd like to see this merged soon (after I add the limits dict to each JSON file).

@jeanconn
Copy link

jeanconn commented Jul 1, 2021

My other recollection was that we hadn't decided if models that weren't in flight use could live in the flight version chandra_models.

@jzuhone
Copy link
Contributor Author

jzuhone commented Jul 14, 2021

@jeanconn the check tools for the BEP and FEP models are installed in ska flight, FWIW.

@matthewdahmer
Copy link
Contributor

I am ok with having non-flight approved models in a flight approved release. We have precedence for this as well. I keep a non-flight approved version of the IPS Fuel Tank model alongside the flight approved version (the non-flight approved version includes the impact of SSR-B, which I took out to generate the current flight approved version).

My perspective is that it is the models that ensure the thermal guidelines are adequately enforced that dictate whether or not a release is "approved". The release tag and commit hash act as a guarantee that each of these models is the correct version. Since each model file spec is separate and does not impact the performance of other models, there is no reason additional models cannot be included.

@jzuhone
Copy link
Contributor Author

jzuhone commented Jul 16, 2021

I've added limits according to the prescription from PR #75, fixed some naming issues in the files, removed the fep1_fb model since it's not used for anything, bumped the version, and rebased on master. It sounds like @matthewdahmer is ok with this going in, but happy to hear from anyone else.

@jzuhone
Copy link
Contributor Author

jzuhone commented Jul 19, 2021

@taldcroft what do you think about this?

@taldcroft
Copy link
Member

taldcroft commented Jul 19, 2021

@jzuhone - putting these models in is fine by me. I guess it would be good to see some minimal statement in the description of internal technical review by the ACIS team for the models:

  • Model predictions are "reasonable" (i.e. someone else on the team has seen these and agrees that going forward with these models is a good thing. No need for plots or quantiles or anything).
  • Limit values are correct

@jzuhone
Copy link
Contributor Author

jzuhone commented Jul 19, 2021

Hi @taldcroft--I added text to the description in line with your suggestions.

@taldcroft taldcroft merged commit ec3fada into sot:master Jul 20, 2021
@jzuhone jzuhone deleted the bep_fep branch July 20, 2021 02:02
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