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

Get ACA penalty and planning limits from chandra_models #356

Merged
merged 2 commits into from
Jun 28, 2021

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jun 24, 2021

Description

This gets the ACA penalty and planning limits from chandra_models. It uses the module-level __getattr__ to make these characteristics be lazily defined to prevent import-time issues.

The current code has been tested using a local version of chandra_models in the SKA data that has a tagged "release" from sot/chandra_models#74 as the default model version. Ideally we should get a new version of chandra_models 3.35.1 promoted that includes only the ACA model update.

Testing

  • Passes unit tests on MacOS
  • Functional testing

@taldcroft taldcroft force-pushed the aca-limits-from-chandra-models branch from 8aa1376 to 94274c8 Compare June 24, 2021 13:41
@taldcroft taldcroft changed the title WIP: get ACA penalty and planning limits from chandra_models Get ACA penalty and planning limits from chandra_models Jun 24, 2021
@jeanconn
Copy link
Contributor

Regarding "The current code is using a local version of chandra_models that has a tagged "release" from sot/chandra_models#74 as the default model version.", do you just mean that in your testing you have a SKA set with a nonstandard version of chandra_models? Or is there something in the code that I'm missing?

@taldcroft
Copy link
Member Author

do you just mean that in your testing you have a SKA set with a nonstandard version of chandra_models?

No, I changed my local "flight" Ska data version of chandra_models for testing purposes. Since (1) chandra_models is not currently used in any flight tools, and (2) the change is innocuous anyway, I was not concerned about this. My local mod will get wiped out the next time I do a ska_sync.

@jeanconn
Copy link
Contributor

I'm not sure how that is a "no"?

@taldcroft
Copy link
Member Author

taldcroft commented Jun 25, 2021

I interpreted a "SKA set" to mean changing my SKA env var to a different value where some test Ska data were located.

@jeanconn
Copy link
Contributor

Gotcha. Your SKA is set to a directory that has a nonstandard chandra_models was my point. Though in your strategy you just used the one that you generally use. Either way, there's nothing funny in the current PR/code to pick up a different version, it is just the test environment.

Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

I like this implementation. Though now that we have a good use case of get_xija_model_spec I'm wondering about the easiest way to specify a nonstandard location of the chandra_models repo for testing. Env vars are annoying but maybe it is worth one?

@taldcroft taldcroft merged commit bff1f93 into master Jun 28, 2021
@taldcroft taldcroft deleted the aca-limits-from-chandra-models branch June 28, 2021 15:06
This was referenced Jun 30, 2021
@javierggt javierggt mentioned this pull request Jul 12, 2021
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