search

sot / xija

Thermal modeling framework for Chandra X-ray Observatory
https://sot.github.io/xija
BSD 3-Clause "New" or "Revised" License
9 stars 5 forks source link

Set the default repo_path inside the get_model_spec function #110

Closed jzuhone closed 3 years ago

jzuhone commented 3 years ago

Description

This PR makes two changes one minor change to xija.get_model_spec. one minor and another that is API-breaking

  1. Let the default for repo_path be None and then set it to REPO_PATH internally, so that calling routines from other packages can pass a None as default and they won't need to know about REPO_PATH. 2. In a couple of cases, I have found that it would be nice to know the path to the JSON file returned by get_model_spec, so it can be reported in logging or print statements. The way I currently do this is rather clunky--I import REPO_PATH from the same module. But this duplicates what's already being done in the function and is error-prone if something changes. So this change simply returns the path as the third argument--unfortunately this also breaks API.

If anyone can think of a non-API-breaking way to achieve the second change, let me know.

jzuhone commented 3 years ago

Pinging @taldcroft @javierggt @jeanconn @matthewdahmer @jskrist (by the way, is it possible for me to be given permission to add reviewers on the right side? Currently I am not allowed)

javierggt commented 3 years ago

Is this change a bugfix that should go into ska3-matlab 2021.7?

jzuhone commented 3 years ago

@javierggt I wouldn't call this a bugfix. it's not urgent. It depends on your timeline though.

jzuhone commented 3 years ago

I really like @jskrist's idea to factor the file path bit out so we don't have to wreck the API. I'll do that instead.

taldcroft commented 3 years ago

Also be aware that it always makes a clone into a temp dir. I didn't look at it very closely but maybe the file path you get will be useless.

jzuhone commented 3 years ago

Maybe we just need to log more info about the model used to the screen?

taldcroft commented 3 years ago

One more comment on the file name is that the basic philosophy behind all this is that the content is what matters and that it lives in a distributed git repo. So the version tag you get back from that repo (e.g. chandra_models version: 3.35.2) is effectively a signed guarantee of the contents.

It probably is not obvious, but you can do somewhat tricky things with this infrastructure. For instance, ACIS could add a brand new model to the repo in a dev branch named bep_fep. Then you could run load review for BEP_PCB from that branch which has files that don't even exist in the flight tag branch.

In [1]: from xija.get_model_spec import get_xija_model_spec                                                                                  

In [2]: spec, version = get_xija_model_spec('bep_pcb', version='bep_fep')                                                                    

In [3]: version                                                                                                                              
Out[3]: 'bep_fep'

Small note before you try this at home, namely that bep_fep branch right now is on the jzuhone remote, so to do this for real we would need to take https://github.com/sot/chandra_models/pull/62 and put that into a sot/bep_fep branch.

jzuhone commented 3 years ago

@taldcroft I see the point now and I am reverting the changes in this PR which involve passing back the model path--I left in the change which sets the default repo_path inside the function so that calling functions can pass None. I'm thinking of the case in which the calling function has its own optional argument for repo_path which may be None or something else (planning this for acis_thermal_check).