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

Move all of the temperature model scripts into the main package #44

Merged
merged 51 commits into from
Nov 24, 2021

Conversation

jzuhone
Copy link
Member

@jzuhone jzuhone commented Nov 6, 2021

Description

This PR moves all of the individual thermal model tools which depend on acis_thermal_check, such as dea_check, acisfp_check, etc., which currently reside in their own packages and github repositories.

This looks incredibly daunting--~over 300 changed files! But most of them are the files which have been added in (scripts, tests, answers) as a result of moving the scripts in. The essential files to review are main.py, state_builder.py, utils.py, index_template.rst, regression_testing.py, and setup.py.

This PR also makes the other changes:

  • Use get_xija_model_spec to get the model specification file
  • Use pathlib instead of os.path
  • Print "hot" ACIS-S observations to the thermal model webpage
  • Change name of SQLStateBuilder to KadiStateBuilder (no functional changes)
  • Minor changes to support the above as needed
  • Support eclipses for model predictions using kadi states

The regression tests which originally resided in the separate packages all passed after moving them to the new package, with minor modifications to the code (changing paths, etc.) but no changes to test answers were necessary. At the end of the development process, when eclipse states were added, this required an update to the test answers. The changes to the test answers were examined and consistent with the adding of eclipses.

A test run of the ACIS "LR" software confirmed that the executable path will not need to be changed once the new acis_thermal_check package is installed.

After the new version of acis_thermal_check is installed in ska, we will need to uninstall the old packages (dpa_check, etc.). I will work with @javierggt , @taldcroft, and @jeanconn to figure this out.

Testing

  • Passes unit tests on MacOS, linux, Windows (at least one required)
  • Functional testing

@jzuhone
Copy link
Member Author

jzuhone commented Nov 7, 2021

A built version of the docs is here:

https://cxc.cfa.harvard.edu/acis/acis_thermal_check_dev/

Copy link
Contributor

@Gregg140 Gregg140 left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

Copy link

@jazan12 jazan12 left a comment

Choose a reason for hiding this comment

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

The 6 files in question look ok to me.

@javierggt
Copy link
Contributor

Before you merge this, I would like to know how to update Ska tests, so I can update them quickly. Tests are currently defined in the test_* files in:

These tests are run:

  • every midnight using all Ska packages in their corresponding master branches.
  • for each Ska release candidate
  • after each release
    but perhaps we can run them at better times for you.

Since these tests take some time to run, I think it is best if you help to determine exactly how we run them, so you also benefit from these extra test runs. Could this save you time in the long run? Would you benefit if these tests are run at other times?

Copy link
Contributor

@Gregg140 Gregg140 left a comment

Choose a reason for hiding this comment

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

Looks good

@jzuhone
Copy link
Member Author

jzuhone commented Nov 23, 2021

@javierggt with this update, all of the tests you mentioned above would now be run from the acis_thermal_check package, and the packages you list would be removed entirely.

@jzuhone jzuhone linked an issue Nov 23, 2021 that may be closed by this pull request
@Gregg140
Copy link
Contributor

Checked the "Support eclipses", "Fixing some bugs" and "Doc updates". All still looks good.

@jzuhone jzuhone merged commit eee5bf1 into acisops:master Nov 24, 2021
@jeanconn jeanconn mentioned this pull request Dec 3, 2021
@jzuhone jzuhone deleted the one_pkg branch February 23, 2022 22:08
@javierggt javierggt mentioned this pull request Aug 3, 2022
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.

should the check scripts come into this repository?
4 participants