-
Notifications
You must be signed in to change notification settings - Fork 171
cam6_3_028: Science Updates and bug fixes for SE dycore. #401
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_028: Science Updates and bug fixes for SE dycore. #401
Conversation
|
Cecile brought to our attention that del4 sponge layer settings are hard coded. We have code to move those settings to namelist. |
|
@PeterHjortLauritzen I have finished my testing of the refactored code that generalizes the sponge layer setup and removes the hard coded layer settings. I will commit the fix for the rest of the reviewers to look at. |
nusbaume
left a comment
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 to me, but I did have some questions and requests.
| <entry id="se_sponge_del4_nu_div_fac" type="integer" category="se" | ||
| group="dyn_se_inparm" valid_values="" > | ||
| Divergence damping hyperviscosity coefficient se_nu_div [m^4/s] for u,v is increased to | ||
| se_nu_p*se_sponge_del4_nu_fac following a hyperbolic tangent function |
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 believe it should be se_nu_p*se_sponge_del4_nu_div_fac 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.
corrected
doc/ChangeLog
Outdated
| If less than 0, se_sponge_del4_nu_fac is automatically set based on model top location. | ||
| se_sponge_del4_nu_div_fac | ||
| . Divergence damping hyperviscosity coefficient se_nu_div [m^4/s] for u,v is increased to | ||
| se_nu_p*se_sponge_del4_nu_fac following a hyperbolic tangent function |
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 believe it should be se_nu_p*se_sponge_del4_nu_div_fac here as well.
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.
corrected
| logical, public :: variable_nsplit=.false. | ||
|
|
||
| integer, public :: phys_dyn_cp = 0 !=0; no thermal energy scaling of T increment | ||
| integer, public :: phys_dyn_cp = 1 !=0; no thermal energy scaling of T increment |
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.
Since you modified the default value of this namelist, I wanted to point out that we are moving towards not having namelist values initialized to anything other than the following (from the Fortran Coding Standards wiki). Something for the next PR.
All namelist variables except for logical quantities are initialized to invalid values (integer: -HUGE(1), real: Nan, character: 'UNSET').
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.
Got it. I'll look over the Fortran Coding Standards wiki before the next PR.
| kmcnd(:ncol,:,lchnk), pcnst, tracer=mmr(:ncol,:,:), fact=to_moist_fact(:ncol,:), & | ||
| active_species_idx_dycore=thermodynamic_active_species_idx) | ||
|
|
||
| cappav(:ncol,:,lchnk) = rairv(:ncol,:,lchnk)/cpairv(:ncol,:,lchnk) |
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.
Sorry we missed this being removed in cam6_2_034. Did it have a big impact on results? This is obviously one of the bug fixes which needs to be applied to CESM2.2.
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.
Yes this is a bug ("only" impacts WACCM-x applications) that needs to be fixed in the release branch and, as far as I understand, will be tackled in a separate pull request. We have not assessed impacts on simulation but the WACCM-x group did a sanity check with the buggy code and did not report issues.
src/utils/physconst.F90
Outdated
| residual = residual - mm | ||
| end do | ||
| icnst=dry_air_species_num | ||
| ispecies = idx_local(icnst) |
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 don't believe you need ispecies here, as it is not being used.
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'll get rid of ispecies here.
nusbaume
left a comment
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 great to me now! Just a few final (very minor) requests I missed the first go-around.
gold2718
left a comment
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 it is important to log namelist values, especially if they are set in the code (see my comment). Did I miss the print statements?
| . bld/build-namelist | ||
| - Remove SE Rayleigh Friction Parameterization |
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.
Totally at your discretion (i.e. no need to change). I typically combine several files that have the exact same description together and then have the description follow. In this case, build-namelist, user_nl_cam, dyn_comp.F90 could all be grouped together with the "Remove SE Rayleigh Friction Parameterization" description.
gold2718
left a comment
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 good now, thanks!
| if (hybrid%masterthread.and.div_set) & | ||
| write(iulog, '(a,e9.2)') ' sponge_del4_nu_div_fac = ',sponge_del4_nu_div_fac | ||
| if (hybrid%masterthread.and.lev_set) & | ||
| write(iulog, '(a,i3)') ' sponge_del4_lev = ',sponge_del4_lev |
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.
FYI, i0 will use the exact needed number of characters to write out an integer value.
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! It is new information to me and I will change to i0.
These mods are necessary to fix science bugs, improve algorithms and numerical stability of SE dycore. Namelist default changes to the gravity wave parameterization and updates to the Test Tracers will also affect the other dycores. This PR will close issue #392.
Improvements:
Bug fixes:
Refactor: