CoMorph KGO-breaking refactoring - #776
Open
MichaelWhitall wants to merge 36 commits into
Open
MichaelWhitall wants to merge 36 commits into
MichaelWhitall wants to merge 36 commits into
Conversation
…ph_refact2 and ran the lfric_styling.py script.
…d parcel radius option, correct sub-grid fraction treatment for graupel in the stand-alone comoprh test.
…amelist block in the rose metadata.
…ck. Try use-ing them from comorph_config_mod?
… case where namelist:convection (and hence namelist:comorph too) are not in brackets as well as the existing brackets case (the ngarch rose-stem job doesn't have the brackets so the existing macro breaks it).
… the match as that's not there in source after parsing by self.get_setting_value).
…the various example configuration .nml files (none of these use comorph so the new namelist doesn't need to be added to any of them.
…ackets even if namelist:convection isn't, to indicate that its optional.
… and just specify the whole string to be added (1 preceding space at start, newline at end) instead of matching the previous line.
…core calculations inside subroutine core_combine, reverted AI changes to comments).
…e newly-added lfric_apps comorph namelist (VScode github copilot helped with this).
…o the namelist (par_radius_knob slightly changes value when read in as 0.45 from the namelist at 32-bit and then converted to 64-bit, compared to when it was hardwired to 0.45 directly at 64-bit).
…ained air properties inside set_ent, renamed hydrometeor species indices in collision_ctl, harmonised some more comments.
…set kind of par_radius_knob to double in the first place, which should mean we no-longer need the check to set it to exactly 0.45 in um_physics_init_mod (so deleted that).
…or code-style / comment changes, (b) moved autoconversion code from microphysics_2 inside its own subroutine, (c) moved setting of core_ent diagnostic from conv_level_step inside set_ent, (d) added / corrected chk_var calls in comorph_um_namelist_mod...
…ctually are (sci/tech review request).
…ibility of multiple condensate species contributing to the cloud-fractions (needed when switching on the 2nd ice category for use with CASIM). Also deleted stray module use of set_par_cloudfrac from conv_level_step (it isn't used in that routine yet).
…t test for phase_change_solve, and a few more code-style changes to harmonize with the comorph_dev branch.
…act3 and ran the LFRic style script.
… double to reproduce KGO in MetOffice#717; now we're breaking the KGOs anyway we may-as-well set this the same as the rest of the comorph namelist for consistency.
…omorph namelist, with sort-keys set to put them next to ass_min_radius.
… whitespace spotted by the lint checker.
MichaelWhitall
added
KGO
github-actions
Bot
added
the
cla-modified
28 tasks
…an the Issue number, for consistency with MetOffice#717 (now that we have the PR number for this one).
|
Your CLA signature was found on the base branch, but you appear to have modified the CONTRIBUTORS.md file in this PR. Please do not edit the CONTRIBUTORS.md file. If you have already signed the CLA, revert changes to the file and your signature will be picked up. |
MichaelWhitall
removed
the
cla-modified
MichaelWhitall
marked this pull request as ready for review
September 11, 2026 23:28
MichaelWhitall
requested review from
a team,
DrTVockerodtMO,
Hacka Fett (christophermaynard),
cjohnson-pi,
iboutle,
James Bruten (james-bruten-mo),
Matt Shin (matthewrmshin) and
Steven Sandbach (ss421)
as code owners
September 11, 2026 23:28
DanStoneMO
approved these changes
Sep 14, 2026
DanStoneMO
left a comment
DanStoneMO
left a comment
Contributor
There was a problem hiding this comment.
Tested with JEDI and it works fine, no linked PR needed for it
iboutle
approved these changes
Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
Sci/Tech Reviewer:
Code Reviewer:
The changes under Issue #178 / PR #292 and Issue #713 / PR #717 left various lines of "temporary code" in place so-as to avoid changing known-good-output under the wholesale refactoring of the CoMorph convection scheme. This included many lines that do nothing scientifically, but are just hacks to force calculations to yield the same floating-point rounding-errors as before. Now we remove these, with no expected science impact, but a KGO-change in CoMorph due to changing rounding-errors in various calculations.
For a detailed description of the changes, see the Issue page: #251
Note: the branch for this PR includes all the changes from #717 as well, since they could not be cleanly separated. Until #717 has been lodged and the head of main has been merged into this PR, the "Files Changed" tab will show the combined diff of this PR and #717. In the mean-time, please review the diff relative to the #717 baseline, not the one in the "Files Changed" tab:
comorph_refact2 -> comorph_refact3
Since we've altered the namelists, this PR requires an upgrade macro to apply the namelist changes to existing apps, and a test branch in-which the upgrade macro has been applied to rose-stem: comorph_refact3 -> comorph_refact3_test
Note since the branch for this PR currently includes namelist changes from both this and #717, there are 2 upgrade macros added in um-convection/versions.py. But I found the
apply_macros.pyscript fails if applying 2 separate macros. Hence the test branch has this file modified to combine the two macros into one. This can be ironed-out once #717 is merged into main and its macro is already applied to all the apps.closes #251
is blocked-by #717
Code Quality Checklist
**Thecheck-clatest is failing, even though I have not modified thecontributors.mdfile in this PR (and my name is already added to it on the trunk). This test just seems to be broken.Testing
**rose-stem KGO-checks fail for all the comorph runs, as expected.Tests of the impact of the changes in UM 10-day AMIP runs are documented here:
Testing CoMorph refactoring and bug-fixes
(equivalent UM branches have been used, so not the actual branch for this lfric_apps PR, but the whole comorph directory is identical between the UM and lfric_apps branches).
The differences for the run with the changes from this PR (
vn14.2refact3_n96_coma9_tb) look like science-neutral / chaotic small deviations in model-evolution as far as I can tell.trac.log
Test Suite Results - lfric_apps - comorph_refact3_test/run1
Suite Information
Task Information
❌ failed tasks - 14
⌛ waiting tasks - 2
Security Considerations
Performance Impact
No change in model performance expected.
AI Assistance and Attribution
A Met Office Github Copilot Enterprise AI agent was used in visual studios code to do extract the relevant existing changes from the UM comorph_dev branch into smaller separate UM branches (vn14.2_comorph_refact2 for #717, vn14.2_comorph_refact3 for #717 + this PR) containing only the refactoring changes (however the agent made many errors so heavy manual intervention has been required after running the agent).
Documentation
Porting comorph documentation to lfric_apps is still tbd, see #359.
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review