-
Notifications
You must be signed in to change notification settings - Fork 318
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
Bug fix for izumi nag tests to pass (PR into tmp-241219) #2925
Bug fix for izumi nag tests to pass (PR into tmp-241219) #2925
Conversation
|
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.
@slevis-lmwg awesome for you finding this. This is another example of Nag finding a legit problem for us.
This is great. Can you also create a branch on b4b-dev where you merge 1c81c98? We can get that into b4b-dev immediately that way.
I still propose we have @glemieux go first with his simple FATES-Hydro tag. And we should have him go now.
@slevis-lmwg I have been having to redo the build and resubmit after it does the initial run through. So that looks the same as what I've been seeing, and also with the ctsm5.3.016 tag on master. |
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.
This is great, and I'm glad you found this.
The one change we need is to add a test where Tom's scheme is turned on. We'll have this when we make it the default, but until then there should be at least one test for it. And actually I suggest we have both a derecho_intel and a izumi_nag test for it now (because of the problem nag found).
Oh, and I suspect that had we had a test for it in the first tag -- we would've seen this problem sooner and with DEBUG for derecho_intel. |
aux_clm results derecho OK |
@glemieux regarding the diffs that I see between this PR (to be ...n03) and ctsm5.3.016:
|
@glemieux my test directory (regarding the above) is |
From my own digging in /glade/campaign/cgd/tss/ctsm_baselines/tmp-241219.n02.ctsm5.3.016,
|
The lists that I posted appear in different order, but I checked them and confirmed that they are identical. |
@slevis-lmwg yep you're correct, these are expected diffs against Reviewing the original diffs ( |
Description of changes
Allocation statements should have been (0:mxpft) instead of (mxpft).
I introduced the bug in a small refactor requested in #2917.
Specific notes
CTSM Issues Fixed (include github issue #):
Fixes #2924
Are answers expected to change (and if so in what way)?
Not relative to ctsm5.3.016.
Testing performed, if any:
I will submit aux_clm next.