-
Notifications
You must be signed in to change notification settings - Fork 145
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
cam6_3_046: PUMAS Updates #409
cam6_3_046: PUMAS Updates #409
Conversation
Change requested by Cheryl made, and I think I updated the code (commit-add-push). Let me know if I did not do this right! |
… for do_implicit_fall
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.
Fix typo (unless I misread it), otherwise okay.
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.
Looks good, although there might be some typos in some history variable descriptions (?).
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.
Like the science behind the changes 1-4. Haven't quite understood 5 (ACCRE). #6 in the document (IFS replication) seems like a good idea but don't know what it entails. Approving PR.
Merge up to most recent Cam development
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.
Still looks good to me! Just two not important nit-picks.
src/physics/cam/nucleate_ice_cam.F90
Outdated
INndust(i,k)=dst_num*1e6_r8/dtime | ||
INondust(i,k)=odst_num*1e6_r8/dtime | ||
INFreIN(i,k)=1.0_r8 ! 1,ice nucleation occur | ||
INhet(i,k) = (niimm(i,k) + nidep(i,k)) ! #/m3, nimey not in cirrus |
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.
Should the comment here say #/m3/s
? Also not sure if the outer parentheses are needed.
INndust(i,k)=dst_num*1e6_r8 | ||
INondust(i,k)=odst_num*1e6_r8 | ||
INFreIN(i,k)=1.0_r8 ! 1,ice nucleation occur | ||
INhet(i,k) = (niimm(i,k) + nidep(i,k)) ! #/m3, nimey not in cirrus |
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.
Not sure if the outer parentheses are needed here?
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 think they are not needed, but also outside of the modifications of this PR.
@Katetc: The description block of this PR does not seem to be formatted in a way that allows GitHub to directly manage the linked issues. Please follow the syntax outlined in the PR instructions (section 4 of submit section, see note about correct syntax). |
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.
Approving, however, the ChangeLog does not look complete. Please address my comments / questions in there.
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.
The following three tests have the error:
[22] /home/katec/sandboxes/PUMASDevelopmentupdater/src/physics/pumas-frozen/micro_pumas_data.F90, line 395: Error occurred in MICRO_
PUMAS_DATA:MGFIELDPOSTPROC_ACCUMULATE
FAIL PLB_D_Ln9_Vmct.ne5_ne5_mg37.QPC5.izumi_nag.cam-ttrac_loadbal0 RUN time=276
FAIL PLB_D_Ln9_Vmct.ne5_ne5_mg37.QPC5.izumi_nag.cam-ttrac_loadbal1 RUN time=561
FAIL PLB_D_Ln9_Vmct.ne5_ne5_mg37.QPC5.izumi_nag.cam-ttrac_loadbal3 RUN time=248
This error was also encountered during the making of #490 and went away when some additional code was added (not related to the MG routine at all).
@KateC said in email that:
" those three tests are failing. They passed before my last commit, which was adding some if-statements in the ice_nucleation code that in no way would change answers for these compsets."
Proposal is to allow this PR to go in with the ACCUMULATE failures as a known issue. The pack/unpack issue #254 should resolve this problem and significant progress has been made on the way to opening a PR for this issue.
Re-review will be asked from the relevant reviewers.
I'm fine with the proposal to move forward with the failing tests. It makes getting rid of the pack/unpack a high priority, but agreed this should not hold up this or other similar work. Thanks! |
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.
Sounds like a reasonable plan to me!
@Katetc, @cacraigucar, The request for a re-review seems premature as my previous comments have not been addressed. Please fix the ChangeLog (especially as we are being asked to review a variance in test procedure). Pleas also fix the description block of this PR so that is formatted correctly for our CI tools. |
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.
Looking much better. ChangeLog still needs a couple of tweaks but that is all I see.
@@ -1,5 +1,128 @@ | |||
=============================================================== | |||
|
|||
Tag name: cam6_3_046 | |||
Originator(s): katec, andrewgettelman | |||
Date: 26 Jan 2022 |
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.
Don't forget to update.
|
||
izumi/pgi/aux_cam: All PASS | ||
|
||
izumi/nag/aux_cam: |
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.
Please add in the failing loadbal
tests to document that they are approved failures and what is going on. This document is the first place a lot of folks look to understand why a test is failing.
Merge pull request ESCOMP#409 from PUMASDevelopment/gettelman_pumas_update0821
Merge pull request ESCOMP#409 from PUMASDevelopment/gettelman_pumas_update0821
This PR addresses a series of updates and bug fixes to the CAM microphysics and ice nucleation schemes.
PR linked to CAM issue #408
Note: this is ready for SE review, but it will need Science Review. I will advise Julio B. of this when the PR is started. Minor code changes. Major science changes.
Repeating some of that information below.
It addresses issues identified in PUMAS issue number 28:
ESCOMP/PUMAS#28
1, fixes issue Possible removal of ice number limiter....doesn't work with new ice nucleation. PUMAS issue number 20:
ESCOMP/PUMAS#20
2. adds in vapor deposition onto snow as a process
3. Fall speed correction for rain/snow/graupel (namelist option turned on)
4. Option for implicit fall speed (namelist option, initially off)
5. Option for the accretion to see newly autoconverted rain (liquid only, namelist option, initially off).
For the moment, 1 and 2 do not have switches, 3-5 have switches (though 3 is a bug fix). They will need science testing, but I have checked the code is functional in SCAM and 3D CAM, and @Katetc has verified it passes all the standard CAM tests (with lots of answer changes).
Fixes #408