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

Add access class for access-esm repro testing #28

Merged
merged 8 commits into from
Jul 8, 2024

Conversation

jo-basevi
Copy link
Collaborator

  • Added an access model class (access is the name of the payu driver for the access-esm1.5 model).

  • Refactor extracting mom5 checksums to a separate file and function so it can be reused in access-om2 and access. I used just a helper function, rather than model class of mom5 with extract_checksums() method to easily allow for different checksums be added to access-om2/access models in future.

  • Added a test access.out and expected checksums to test resources so it passes the existing extract_checksums() tests for each model class.

  • Set default model runtime for access to 24hr for access (as the minimum runtime is day), and keep access-om2/om3 to 3hrs. This default runtime is used in the historical repro checksums tests.

  • Refactored the test_bit_repro_historical a little as the test was getting long with the added logic..

  • Added a start of an integration-like test for test_bit_repro_historical. This is still draft while I am finishing off adding tests. But if there's any feedback on the changes is welcome

ACCESS-ESM1.5 default model runtime is a day as UM payu model driver only supports days,
while ACCESS-OM2 default model runtime is kept at 3hrs. The output checksum files are still named as historical-*hr-checksums.json
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 87.20000% with 16 lines in your changes missing coverage. Please review.

Project coverage is 54.85%. Comparing base (c9d6184) to head (13472bf).
Report is 9 commits behind head on main.

Files Patch % Lines
src/model_config_tests/exp_test_helper.py 47.36% 10 Missing ⚠️
src/model_config_tests/models/accessesm1p5.py 91.42% 3 Missing ⚠️
src/model_config_tests/test_bit_reproducibility.py 94.28% 2 Missing ⚠️
src/model_config_tests/util.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #28       +/-   ##
===========================================
+ Coverage   14.44%   54.85%   +40.41%     
===========================================
  Files          11       13        +2     
  Lines         547      618       +71     
===========================================
+ Hits           79      339      +260     
+ Misses        468      279      -189     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jo-basevi
Copy link
Collaborator Author

  • Added more tests for test_bit_repro_historical. It's somewhat ironic having a lot of test code for tests.. The tests probably can be improved and broken up to test more smaller functions that are moved outside of test_bit_reproducibility.py and just have one or 2 integration style tests. In future, I want to test the other repro and config pytests, and other models rather than just access. Though that might be moving out of the scope of this PR.
  • Added a quick improvement to the logging when the payu run fails in test_bit_repro_historical. I could add more logging, and use the logging library but I'll save that for an PR of this issue: Further Logging of pytest States #5

@jo-basevi jo-basevi marked this pull request as ready for review July 3, 2024 06:07
@CodeGat CodeGat self-requested a review July 3, 2024 06:26
CodeGat
CodeGat previously approved these changes Jul 4, 2024
Copy link
Collaborator

@CodeGat CodeGat left a comment

Choose a reason for hiding this comment

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

Wow, from all of that I only have one comment 😄 Looks great.

Have you tested the test code?

src/model_config_tests/models/access.py Outdated Show resolved Hide resolved
@jo-basevi
Copy link
Collaborator Author

I've tested the code with the pre-industrial configuration: https://github.com/ACCESS-NRI/access-esm1.5-configs/tree/pre-industrial, but using the executable paths from coecms: https://github.com/coecms/access-esm/blob/main/config.yaml, as the exe paths on the current pre-industrial branch weren't working.

Though I think the pre-industrial configuration is getting updated with spack executables, so I'll be able to do another test run.

I've tested it with the broken 025 access-om2 configs tests to see what logs are printed when things go wrong..

@jo-basevi
Copy link
Collaborator Author

Ok, so the spack executables in this PR: ACCESS-NRI/access-esm1.5-configs#26, pass the historical bit reproducibility test using checksums from the coecms executables as a reference.

Restart reproducibility fails, because the model is not in a good state to restart from after a model runtime of one day - it's minimum runtime might be a month (?). Talked with @aidanheerdegen about whether restart reproducibility should be tested as part of Reproducibility CI checks for configs (as they fail for the access-om2-bgc configs too), and decided that bit historical reproducibility was sufficient testing for now. So I've changed the test marker to checksum_slow, and will create a separate issue for refactoring the restart repro tests. Does that sound an ok plan?

@aidanheerdegen
Copy link
Member

Ok, so the spack executables in this PR: ACCESS-NRI/access-esm1.5-configs#26, pass the historical bit reproducibility test using checksums from the coecms executables as a reference.

Hey that's cool!

Restart reproducibility fails, because the model is not in a good state to restart from after a model runtime of one day - it's minimum runtime might be a month (?). Talked with @aidanheerdegen about whether restart reproducibility should be tested as part of Reproducibility CI checks for configs (as they fail for the access-om2-bgc configs too), and decided that bit historical reproducibility was sufficient testing for now. So I've changed the test marker to checksum_slow, and will create a separate issue for refactoring the restart repro tests. Does that sound an ok plan?

I believe ESM1.5 has never been restart reproducible. @MartinDix mentioned that CABLE (the land model embedded in the UM in ESM1.5) also prevented restart reproducibility, so best to not have it turned on as a check. Which also means we can just use 1 day runs for historical reproducibility.

aidanheerdegen
aidanheerdegen previously approved these changes Jul 5, 2024
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

Just a warning, this was a pretty light review, I didn't have time to check all the refactoring, but it looks great to me. Thanks for all the hard work!

src/model_config_tests/models/access.py Outdated Show resolved Hide resolved
src/model_config_tests/models/access.py Outdated Show resolved Hide resolved
src/model_config_tests/test_bit_reproducibility.py Outdated Show resolved Hide resolved
tests/test_test_bit_reprodubility.py Show resolved Hide resolved
tests/resources/access/output000/access.out Outdated Show resolved Hide resolved
tests/test_test_bit_reprodubility.py Show resolved Hide resolved
tests/test_test_bit_reprodubility.py Outdated Show resolved Hide resolved
@jo-basevi
Copy link
Collaborator Author

I reran the historical repro test with ACCESS-ESM1.5 pre-industrial config and an ACCESS-OM2 config, just to make sure the latest changes didn't break the tests.

@CodeGat CodeGat self-requested a review July 8, 2024 04:36
Copy link
Collaborator

@CodeGat CodeGat left a comment

Choose a reason for hiding this comment

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

Just one thing... 😄

src/model_config_tests/util.py Outdated Show resolved Hide resolved
tests/resources/access/output000/access.out Outdated Show resolved Hide resolved
@jo-basevi jo-basevi merged commit 42c65a8 into main Jul 8, 2024
8 checks passed
@jo-basevi jo-basevi deleted the 21-Add-mom5-and-access-model-class branch July 8, 2024 05:08
@jo-basevi jo-basevi mentioned this pull request Jul 8, 2024
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.

3 participants