Jules red veg3 couple phenology - #160
Arthur Argles (mo-arthurargles) wants to merge 12 commits into
Conversation
…ice#90) - Add veg3_phenol_couple to run phenology on its own timestep and diagnose the mean phenology-driven leaf turnover rate driving litter/RED dynamics, replacing the lai_bal-only estimate - Thread a_step and new phenology period controls (nstep_phen, dt_phen_360d) through veg3_ctrl/next_gen_biogeochem - Use veg3 lai/canht/leafC/woodC/lai_bal in sparm, cancap and sf_stom instead of TRIFFID lai_bal estimates when l_red is set - Add pointer associations for new veg3/RED diagnostic outputs (npp_dr_out, g_leaf_phen, g_leaf_phen_acc, lai_phen, litter fluxes) into trifctl/trif_vars - Refactor veg3_red_dynamic_mod, splitting growth_onto_mass_class and update_pft_size_structure out of veg3_red_dynamic - Fix mclass_geom_mult default and mort_base unit conversion in veg3_param_mod Refs MetOffice#90
…state. Deleted redundant bits of code we don't need to get phenology correctly estimated in veg3_field_mod.F90. Tests now pass umpd3_checker and rose stem tests.
github-actions
Bot
added
the
cla-required
github-actions
Bot
added
cla-signed
|
Hello, Looking for module approvals for vegetation (Karina Bett-Williams (@kwilliams-mo), Becky Oliver (@beckyoliver)) and veg3/Demography (andy-wiltshire) - I'm not 100% if I could approve this myself for veg3. Arthur |
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and robustness issues in the updated litter/demography coupling (including array shape safety and carbon-balance handling) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR couples the existing JULES phenology scheme into the veg3/RED (ecosystem demography) pathway, refactors the RED demographic update for modularity, and extends initialization/diagnostics so veg3/RED can be compared more directly with the legacy veg2/TRIFFID workflow.
Changes:
- Add a veg3 phenology coupling path with an independent phenology timestep and a phenology-driven leaf turnover rate used by litter/dynamics.
- Refactor RED demographic update into helper subroutines and introduce mortality/demographic litter diagnostics feeding into veg3/JULES diagnostics.
- Introduce veg3 field association in initialization and update surface coupling to use LAI (rather than lai_bal) under RED.
File summaries
| File | Description |
|---|---|
| src/science/vegetation/veg3_red_dynamic_mod.F90 | Refactors RED demographic update and outputs mortality/demographic litter diagnostics. |
| src/science/vegetation/veg3_litter_mod.F90 | Updates litter calculation to use phenology-driven leaf turnover and writes litter diagnostics. |
| src/science/vegetation/phenol_jls.F90 | Allows phenology to use a veg3-provided balanced LAI when running RED. |
| src/science/surface/sf_stom_jls_mod.F90 | Uses veg3 balanced LAI / wood carbon paths under l_red for nitrogen-related calculations. |
| src/science/surface/cancap_jls.F90 | Uses veg3 carbon pools directly for canopy heat capacity under l_red. |
| src/science/params/veg3_param_mod.F90 | Adds phenology period controls to veg3 control structure and adjusts parameter unit conversions. |
| src/initialisation/standalone/init_vars_tmp_mod.F90 | Avoids resetting TRIFFID diagnostic accumulators when running RED. |
| src/initialisation/standalone/init_mod.F90 | Calls veg3_field_assoc during init to connect veg3/RED fields to the broader model. |
| src/control/shared/veg3_field_mod.F90 | Expands veg3 state/diagnostics, adds association routine, and updates RED↔veg3 coupling logic. |
| src/control/shared/surf_couple_extra_mod.F90 | Passes a_step through to veg3 driver and switches surface coupling to use LAI under RED. |
| src/control/shared/next_gen_biogeochem_mod.F90 | Adds phenology coupling/control flow and updates veg3 runtime sequencing and diagnostics. |
| CONTRIBUTORS.md | Adds the contributor entry for the author. |
Review details
Suppressed comments (1)
src/control/shared/veg3_field_mod.F90:466
frac_oldis documented as being used to weight litter fluxes over the previous canopy fraction, but it is only assigned and never used afterwards. This is dead code at present and may indicate a missing step in the litter aggregation logic.
! Record the vegetation fraction prior to updating it below, for use in
! weighting litter fluxes that occurred over the previous fraction.
frac_old(:,:) = veg_state%frac(:,1:nnpft)
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hi Arthur Argles (@mo-arthurargles). I'm pretty sure that you'll need to run UM & LFRic apps Rose stem tests as shared code is modified. |
1. In phenol_jls.F90 set in variable lai_bal_red to be be REAL(KIND=real_jlslsm) to ensure consistency between other REAL inputs. 2. From veg3_field_mod.F90, remove frac_old from red_veg3_couple_mod.F90 not necessary for this change and not used in the script.
…ed routines related to veg3_field_mod.F90.
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed large-memory leak risk from allocating POINTER components that are later pointer-associated (notably plantNumDensity) and a small but concrete units/documentation inconsistency that should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/control/shared/veg3_field_mod.F90:250
- veg3_field_allocate allocates many POINTER components (including red_state%plantNumDensity) that are later re-associated in veg3_field_assoc, which leaks the original allocations (and can be large for plantNumDensity). Deallocate the locally-allocated pointer targets before re-associating them (or avoid allocating them in the first place).
IF (l_red .AND. l_triffid) THEN
! Set pointers to the prognostic fields
! Note: veg_state%phen is not associated to an external target here - it is
! not an independent prognostic. It is diagnosed each phenology call as
! lai / lai_bal (see veg3_phenol_couple in next_gen_biogeochem_mod), so it
! remains the array allocated locally in veg3_field_allocate.
red_state%plantNumDensity => progs%plantNumDensity
veg_state%frac => ainfo%frac_surft
veg_state%canht => progs%canht_pft
veg_state%lai => progs%lai_pft
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
…fice#90). Added veg3_field_deallocate during veg3_field_assoc, such that we avoid potential memory issues with pointer when associating with prognostic & diagnositic variables. Fixed typos and inconsistencies with mort_litC description.
There was a problem hiding this comment.
🟡 Changes recommended
Confirmed carbon-accounting and scaling bugs were introduced in the RED demographic litter handling and veg3 phenology initialisation logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/control/shared/veg3_field_mod.F90:409
- During initialisation, if
lai(l,n) > lai_bal(l,n)this code setsphento ~0 (TINY(0.0)), which will collapse LAI after the subsequentred_veg3_couplecall. The established TRIFFID behaviour is to cappheninto [0,1] (i.e., treatlai > lai_balasphen=1).
src/science/vegetation/veg3_red_dynamic_mod.F90:146
mort_litCis renormalised byveg_state%frac(l,n)immediately afterupdate_pft_size_structure, but that fraction is from the pre-dynamics state. Sinceupdate_pft_size_structurecan changeplantNumDensity(and therefore the implied PFT fraction), this will mis-scale mortality litter (and downstreamlitCpft) whenever the PFT fraction changes.
! Divide mort_litC by the PFT fraction (not re-estimated yet)
IF (veg_state%frac(l,n) > 0.0) THEN
veg_state%mort_litC(l,n) = veg_state%mort_litC(l,n) / &
veg_state%frac(l,n)
ELSE
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
Hi Maggie (Maggie (@maggiehendry)), I've now added in trac.logs for both UM & LFRic apps for the rose stem developer tests. |
|
Trying to get module approvals sorted, andy-wiltshire has given sign off for the veg3 demography side of things. I've written a description with links to the relevant files modified for the code. Surface (John Edwards (@JMEdwardsXtr))
Vegetation (Becky Oliver (@beckyoliver))
Biogeochemistry (Noah Smith (@noahdsmith) or Eleanor Burke (@eleanorgb))
Some of these are very small changes just touching on code owners / module leaders. This is based on Maggie (@maggiehendry) documentation for the .github/CODEOWNERS change in /pull/146 where module leaders have been attributed to files. |
Approved. |
…nover (g_leaf, g_leaf_day, g_leaf_dr_out). Related issue MetOffice#90.
|
I've now added more diagnostics outputs for leaf turnover such that we match what we expect from TRIFFID: g_leaf, g_leaf_dr_out, and g_leaf_day. g_leaf_day and g_leaf_dr_out is especially important if we want to see the effects of having different veg dynamics timestepping to the phenology. Rerunning both u-ec255 and the necessary rose stem tests. |
…agnostic purposes, while having a seperate "g_leaf_dr" in turnover per second.
Becky Oliver (beckyoliver)
left a comment
Becky Oliver (beckyoliver)
left a comment
There was a problem hiding this comment.
This looks fine to me.
PR Summary
Issue #90 - Couples the existing JULES phenology scheme to the veg3/RED component.
Sci/Tech Reviewer:
Code Reviewer: Pierre Siddall (@Pierre-siddall)
This branch couples the phenology routines to the veg3/JULES component and adds the necessary changes to the initialisation and diagnostics for a reasonable scientific comparison between the older veg2/TRIFFID veg dynamic scheme and RED.
Additionally, we refactor and have tidied some of the RED dynamic and veg3 coupling code for easier future developments.
Code Quality Checklist
(Some checks are automatically carried out via the CI pipeline)
rose-meta/jules-sharedthen have you supplied a linked UM and LFRic Apps PR?Testing
We still need to develop and add a rose-stem test for veg3 scheme. Linked issue here is #100. However, we need a good scientific basis before this.
I have created an evaluation suite, u-ec255@366212 for a temperate location in Belgium. A list of the following tests:
There is an attached presentation (JULES_RED_Phenology_evaluation.pptx) detailing the comparison of JULES-RED and JULES-TRIFFID, the key findings are:
trac.log
Test Suite Results - jules - jules_red_veg3_couple_phenology/run1
Suite Information
Task Information
✅ succeeded tasks - 676
Test Suite Results - um - jules_red_veg3_couple_phenology_um_test/run1
Suite Information
Approvals
Code Owners
Config Owners
No UM Config Owners Required
Task Information
✅ succeeded tasks - 895
Test Suite Results - lfric_apps - jules_red_veg3_couple_phenology_lfric_apps_test/run1
Suite Information
Task Information
✅ succeeded tasks - 1216
Security Considerations
Performance Impact
AI Assistance and Attribution
The Met Office GitHub Copilot Enterprise License has been used to help refactor functions linked to veg3 routines red_veg3_couple.F90 (growth_onto_mass_class and update_pft_size_structure) and veg3_field_mod.F90 (pft_mean_from_mass_class). Copilot has been used to help have separate timesteping calls for phenology and the vegetation dynamics in surf_couple_extra_mod.F90, veg3_param_mod.F90 and veg3_litter_mod.F90. In veg3_field_mod.F90 used to address reviewer comment for disallocation of pointer arrays before reassignment to prognostic/diagnostic variables.
Documentation
Approvals
Please request all relevant approvals. See the CodeOwners.txt file for section owners.
Technical
Scientific
Sci/Tech Review
Please alert the code reviewer via a tag when you have approved the SR
Code Review