Accurately-reversible PC2 homogeneous forcing option#406
Accurately-reversible PC2 homogeneous forcing option#406MichaelWhitall wants to merge 19 commits intoMetOffice:mainfrom
Conversation
| =Analytical solution | ||
| values='explicit','implicit','analytic' | ||
|
|
||
| [namelist:cloud=i_pc2_homog_g_method] |
There was a problem hiding this comment.
I might suggest losing the "i_" from the start of this - we typically don't want to follow the UM in prefacing things with i_ and l_ anyway (their type is documented by the metadata), but that is especially so when this is no longer an integer, but is now an enumeration!
There was a problem hiding this comment.
Haha! Just because something was common-practice in the UM doesn't mean its automatically bad does it??
I've carried on using the preceding l_s and i_s in namelist switches (and I'm definitely not the only one!) because its quite handy to be able to tell at a glance (when looking at a list of namelist inputs) which ones are switches (l_ for on/off, i_ for multi-option), as opposed to model free parameters. I know that information is all in the meta-data, but having to go and look it up in there is much more time-consuming than using l_s and i_s. Also this naming convention keeps the switches grouped together in the alphabetically-ordered rose-meta.conf and namelist files, making them easier to search.
It doesn't matter that the multi-option switches aren't labeled using integers in the namelist anymore; the point is everyone knows that i_ denotes a multi-option switch (and actually under the hood they still are integers really!)
In my case there's also the challenge of keeping comorph configurations traceable between the UM and LFRic; keeping that traceability is much easier if the namelist inputs have the same name in both models. So keeping the i_ would make things easier?
There was a problem hiding this comment.
I think we did have guidance discouraging it in the early days, although as you point out, many have snuck through somehow! I don't really have an issue with the use of l_ for logicals or i_ for integers, but what I would like to avoid is the use of i_ for enumerations - they are different to integers (even if they use similar machinery under the hood). I see 2 problems with it:
- it's assuming knowledge on behalf of users that i_ means multi-option switch, which deviates from common practice that i_ means integer
- it confuses enumeration inputs with genuine integer inputs which use i_ (of which there are some), when these things would be better to clearly distinguish and separate
It should still be obvious from the variable name what it is - in this case pc2_homog_g_method identifies it as a switch (you wouldn't name anything else method), and just losing the i_ but keeping the name the same allows for easy cross-referencing with the UM, and is what we've done with other switches (e.g. pc2_init_logic).
There was a problem hiding this comment.
OK I can change i_pc2_... to pc2_... if you're sure that's worth spending the time on? If we're going to do that though, I should probably also change the names of the other existing i_ switches in the cloud-scheme namelist to make it all consistent? (easy enough to add stuff in the upgrade macro to change the names in all upgraded configs too of course). Are you happy for me to lump that in with this PR?
Cheers!
Mike
There was a problem hiding this comment.
I think just fix for your new switch here - I'm happy to pick up the rest on a PR I have which is adding quite a lot of new stuff to the namelist, given I'm the one being grumpy about it!
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
macro approved
…moving UM-isms, including preceding 'i_'s on multi-option switches.
…moving UM-isms, including preceding 'i_'s on multi-option switches.
PR Summary
Sci/Tech Reviewer:
Code Reviewer: Benjamin Went (@MetBenjaminWent)
In the PC2 cloud-scheme, many processes which modify the grid-mean Relative Humidity alter the liquid cloud water content and fraction by assuming the process acts homogeneously over the grid-box, shifting the mean of the sub-grid moisture PDF without changing its shape. These increments to liquid-cloud water and fraction are calculated by the many calls to PC2 homogeneous forcing subroutines across the model. This calculation has to make some assumptions about the sub-grid moisture PDF, so-as to reconstruct it from only the prognosed grid-mean cloud water and fraction.
Under balanced forcings from different processes (e.g. resolved-scale uplift balanced by "compensating subsidence" from the convection scheme), where the grid-mean Relative Humidity is increased by one process but decreased by another with zero net change, the cloud amount ought to remain constant under the assumed homogeneous forcing. However, the time discretisation used in the existing homogeneous forcing calculations introduces numerical errors which cause the cloud amount to drift when it should be in balance.
We now introduce an accurately reversible time discretisation to eliminate spurious drift in cloud amount.
While we're here, this PR also tidies-up the rose meta-data for the cloud namelist, removing confusing UM-isms from the help text and consistently renaming multi-option switches without the preceding
i_that was used in the UM namelists.Branch at vn3.1: vn3.1_pc2_homog_reversible
Test branch: pc2_homog_reversible_test
closes #247
Code Quality Checklist
****I have checked for compiler warnings output by each file I modified in thejob.errfile from thebuild_lfric_atm_azspice_gnu_full-debug-32bitcompile in rose-stem. No warnings were found for the modified files.**Thecheck_cr_approvedCI task clearly won't succeed until code review is approved.Testing
**NA*I have switched the new functionality on in the existingcomorph_devrose-stem app for testing, so the rose-stem tests of this app fail the KGO checks by design. In addition, KGO-checks have failed for CCE-fast 64-bit compilelfric_atmglobal jobs. The equivalent jobs at 32-bit preserve KGO. The change in answers at 64-bit is presumably due to compiler optimisations harmlessly changing behaviour of the existing code around my modifications.trac.log
Test Suite Results - lfric_apps - pc2_homog_reversible_test/run1
Suite Information
Task Information
❌ failed tasks - 7
⌛ waiting tasks - 2
Note: I have intentionally left the update of KGO checksums out of the test branch, so that those tests which change KGO show-up above as failures in the
check_lfric_atmtasks for clarity. These tasks would not fail on the dev branch as the KGO files have been updated there.Security Considerations
NANAPerformance Impact
This PR adds new functionality under a switch / option; not expecting any noticeable impact on performance when the new functionality is off.
AI Assistance and Attribution
Documentation
Documentation pending...
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review