search

ACCESS-NRI / model-config-tests

Tests for checking model configurations
Apache License 2.0
0 stars 1 forks source link

Add AccessOm3 model class #19

Closed dougiesquire closed 4 months ago

dougiesquire commented 5 months ago

This PR adds a very basic AccessOm3 model class. I wrote this a while ago and I see that in the meantime a check_checksums_over_restarts method has been added to the AccessOm2 class. From a very quick glance, it looks like the AccessOm3 implementation of this method would be the same. So could this method implementation be moved to the Model base class?

I haven't added any tests to the test directory but probsibly should. Lemme know.

Closes #7

aidanheerdegen commented 5 months ago

This is the PR I mentioned in the stand up this morning @jo-basevi. I've tagged you for a review.

dougiesquire commented 5 months ago

Yeah, maybe the access-om2 schema could be the default checksum schema? And then it would make sense for check_checksums_over_restarts be the default method in the Model base class? Or alternatively, check_checksums_over_restarts be an external function in accessom2 that can be imported to accessom3

Sounds good to me 😄

If there's an example file for ocean.stats and an expected checksum value for it, I am happy to add in a test for it.

I've added an example ocean.stats and access-om3-checksums-1-0-0.json to test/resources. Would be great if you could write/refactor the tests thanks! (feel free to add to this PR)

jo-basevi commented 4 months ago

Firstly, sorry for force pushing - I only realised after a git push that I needed to amend a commit message..

So I've updated the extract_checksums tests, it was very good because I noticed that schema has an error which will need to be fixed. I've added that and moving the schema to a more general location in this PR: https://github.com/ACCESS-NRI/schema/pull/24

Once, that is merged I can update the URL to point to a new location and commit, as the url is not used anywhere apart from the tests for extract_checksums under test.

As part of package tests issue #3, it'll will be good to add in CI to automatically run these tests so it is actually checking the schema validation.

dougiesquire commented 4 months ago

So I've updated the extract_checksums tests, it was very good because I noticed that schema has an error which will need to be fixed. I've added that and moving the schema to a more general location in this PR: https://github.com/ACCESS-NRI/schema/pull/24

Thanks @jo-basevi. Is the idea to merge https://github.com/ACCESS-NRI/schema/pull/24 first so that we can get the tests running here before merging? Or is the plan to update the schema in a subsequent PR (and merge this with failing tests)?

jo-basevi commented 4 months ago

It would be great to merge the schema PR first. But if its needed to merge this to main asap, I can create a separate issue for updating schema url once the schema PR is merged.

dougiesquire commented 4 months ago

But if its needed to merge this to main asap

Nope, I'm not in any particular rush. I agree it would be good to have working tests before merging

dougiesquire commented 4 months ago

@aidanheerdegen looks like we need you to approve (if you're happy to)

jo-basevi commented 4 months ago

I'll rebase this branch onto changes from main as I imagine there's going to be a few merge conflicts

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 56.33803% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 14.36%. Comparing base (dff16de) to head (213dd71). Report is 1 commits behind head on main.

Files Patch % Lines
src/model_config_tests/models/accessom3.py 65.95% 16 Missing :warning:
src/model_config_tests/models/model.py 33.33% 12 Missing :warning:
src/model_config_tests/models/accessom2.py 25.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #19 +/- ## ========================================= + Coverage 9.18% 14.36% +5.18% ========================================= Files 10 11 +1 Lines 501 550 +49 ========================================= + Hits 46 79 +33 - Misses 455 471 +16 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jo-basevi commented 4 months ago

I ran a test on gadi to double check I didn't mess up the merge, and can confirm no errors when testing a released access-om2-configs configuration.