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

ACCESS-OM3: use per-variable checksums from mom restart files #110

Merged
merged 3 commits into from
Feb 26, 2025

Conversation

dougiesquire
Copy link
Collaborator

@dougiesquire dougiesquire commented Feb 21, 2025

This PR modifies the AccessOm3 test class to extract per-variable checksums from MOM restart files, rather than using the ocean.stats output. This required some minor modifications to the ExpTestHelper and base Model test classes to allow the archive restart directories to be easily used.

Relevant to #86, although doesn't actually use the Payu hashes as suggested in that issue. See COSIMA/access-om3#278 for some additional context/discussion.

Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.88%. Comparing base (c4a40b3) to head (dfcaa49).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/model_config_tests/models/accessom3.py 96.15% 1 Missing ⚠️
src/model_config_tests/test_bit_reproducibility.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   72.77%   74.88%   +2.11%     
==========================================
  Files          17       17              
  Lines         889      900      +11     
==========================================
+ Hits          647      674      +27     
+ Misses        242      226      -16     

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

@dougiesquire dougiesquire force-pushed the 86-better-om3-checksums branch from 0f142bd to 399e8d0 Compare February 23, 2025 22:30
@dougiesquire dougiesquire marked this pull request as ready for review February 24, 2025 04:30
Copy link
Contributor

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

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

I haven't run this yet but will soon. I left a bunch of fairly inconsequential comments on the tests

I think you could leave the issue open because this only addresses MOM repro, not other components

Copy link
Collaborator

@jo-basevi jo-basevi left a comment

Choose a reason for hiding this comment

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

Just confirming that I've looked at the code and everything added looks good to me. I haven't got any additional feedback that hasn't been brought up already

@anton-seaice
Copy link
Contributor

I ran this with access-om3 at it works as expected.

Run 1: make checksums and then I copied them into testing/checksums
Run 2: pass
Run 3: change MOM_input and confirmed fail correctly

restart repro from cold start passed and 2x1day with restart in the middle compared to 1x2 day failed, consistent with other tests

@dougiesquire dougiesquire force-pushed the 86-better-om3-checksums branch from bd3a875 to 239dd8f Compare February 25, 2025 09:54
@dougiesquire
Copy link
Collaborator Author

@anton-seaice (and/or anyone else) could you take another look please? Once you're happy I'll tidy up the history a little before merging

anton-seaice
anton-seaice previously approved these changes Feb 26, 2025
Copy link
Contributor

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

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

Thanks @dougiesquire !

@dougiesquire dougiesquire force-pushed the 86-better-om3-checksums branch from ad126bf to dfcaa49 Compare February 26, 2025 22:46
@dougiesquire
Copy link
Collaborator Author

@anton-seaice, I tidied up the history a little. Could you please approve again

Copy link
Contributor

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

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

Thanks @dougiesquire

@dougiesquire dougiesquire merged commit 25bcb61 into main Feb 26, 2025
8 checks passed
@dougiesquire dougiesquire deleted the 86-better-om3-checksums branch February 26, 2025 22:50
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.

4 participants