-
Notifications
You must be signed in to change notification settings - Fork 26
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 checks of io layout to cmeps driver #496
Conversation
Hello @anton-seaice! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-09-18 06:20:40 UTC |
Apologies to the wrong minghangli !! @minghangli-uni, can you look at this please? |
Thanks @anton-seaice. I just did |
I've done a few tests on
What I've found is,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this @anton-seaice. Now that we're starting to accumulate a few checks in the driver, I think it would be worth putting these in a new _qa_check
(or something) method that's called from setup
for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anton-seaice for your work! I've left some comments. Can you please take a look at those?
|
7b9bff6
to
5facb60
Compare
@minghangli-uni - Are you able to review this please? I updated the initial comment with the scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anton-seaice for implementing this check. I have some comments and suggested changes that need to be addressed.
I havent checked the tests yet.
Co-authored-by: minghang.li <minghangl1101@gmail.com>
@aidanheerdegen - this is ready to go. Can you or one of the release team want to review ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some questions and suggestions.
Co-authored-by: Aidan Heerdegen <aidan.heerdegen@anu.edu.au>
Before I forget when this does get merged please either squash when merging, or rebase beforehand to clean up the commit history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have pinged @minghangli-uni because there are some unresolved conversations that he should check have been resolved and mark them as such. Then good to merge I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @anton-seaice
This PR implements checks of the processors selected in the nuopc.runconfig file for the access-om3 payu driver. This ensures that the processors requested for normal model operations and parallel IO are within range of the CPUs set in config.yaml and each model "realm" uses processors for IO that are within the range of processors for that realm.
It adds tests for the min/max bounds of each parameter.
Contributes to COSIMA/access-om3#109