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

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

Merged
merged 6 commits into from
Mar 6, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jan 24, 2023

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

  • Mac

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

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

With this update the unit tests passed.

@jeanconn
Copy link
Contributor

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.

chandra_aca/star_probs.py Outdated Show resolved Hide resolved
@taldcroft
Copy link
Member Author

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

Genius, great catch!

@taldcroft taldcroft changed the title Read grid acq probability model data from in SKA/data/chandra models Read grid acq probability model data from in chandra models repo Jan 24, 2023
@taldcroft
Copy link
Member Author

@jeanconn - ready for review again.

@taldcroft taldcroft requested a review from jeanconn January 24, 2023 16:36
@jeanconn
Copy link
Contributor

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 (

models = ["sota", "spline", "grid-floor-2018-11"]
) but do have an independent test (
def test_grid_floor_2020_02():
) 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
Copy link
Contributor

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

@taldcroft taldcroft merged commit fead981 into master Mar 6, 2023
@javierggt javierggt mentioned this pull request Jul 12, 2023
@jeanconn jeanconn deleted the acq-prob-model-in-chandra-models branch August 7, 2023 15:22
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