search

sot / chandra_aca

Chandra Aspect Camera Tools
https://sot.github.io/chandra_aca
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Read grid acq probability model data from in chandra models repo #140

Closed taldcroft closed 1 year ago

taldcroft commented 1 year ago

Description

In order to facilitate more agile updates of the acquisition probability model, this PR takes the grid data file out of this repo and instead reads it from the chandra_models repo. The location of that repo is delegated to functions in ska_helpers.paths.'

This also now uses ska_helpers.paths to get the location of the aca_drift model from chandra_models.

Related required PR's

Interface impacts

None

Testing

Unit tests

Independent check of unit tests by [REVIEWER NAME]

Functional tests

On my Mac, I checked out the $SKA/data/chandra_models repo to the aca-acq-prob branch from https://github.com/sot/chandra_models/pull/100.

With this update the unit tests passed.

jeanconn commented 1 year ago

I'm wondering if it makes sense to have this compatible with THERMAL_MODELS_DIR_FOR_MATLAB_TOOLS_SW aka https://github.com/sot/chandra_aca/pull/135/files in the first version or if the plan will be to transition to having the matlab tools supply filename before this is included in a matlab tools release.

taldcroft commented 1 year ago

I'm wondering if it makes sense to have this compatible with THERMAL_MODELS_DIR_FOR_MATLAB_TOOLS_SW

Genius, great catch!

taldcroft commented 1 year ago

@jeanconn - ready for review again.

jeanconn commented 1 year ago

Overall, I was trying to figure out if the testing for star_probs is sufficient for the relocation of the grid model files. I think it is OK? The star probs tests don't have the 2020-02 grid data in the regression data (https://github.com/sot/chandra_aca/blob/226eee3d48c131261166d628522ff9b738802cdc/chandra_aca/tests/test_star_probs.py#L44) but do have an independent test (https://github.com/sot/chandra_aca/blob/226eee3d48c131261166d628522ff9b738802cdc/chandra_aca/tests/test_star_probs.py#L498) that is working. This PR might also warrant something like the monkeypatch test you made for the drift model (once that is updated to work with the new stuff in ska_helpers).

jeanconn commented 1 year ago

Per conversation, we don't need equivalent monkeypatch testing of "how do you set where the model comes from" stuff for the acq probability as the cases are handled in https://github.com/sot/ska_helpers/pull/30/files#diff-2dabbe1538d162ce5904aeb803b58c266799f45f8c825c7d734c6344585ad807