61 extracting inferno hardwired flammability params - #145
Eleanor Burke (eleanorgb) wants to merge 34 commits into
Conversation
github-actions
Bot
added
cla-required
There was a problem hiding this comment.
Pull request overview
This PR restructures JULES fire-related configuration by introducing a dedicated jules_inferno namelist (and module) for INFERNO/TRIFFID fire parameters, while also refactoring several standalone/ancillary routines into proper Fortran modules and tightening some CI/workflow configuration.
Changes:
- Added
jules_inferno_modand migrated INFERNO/TRIFFID fire switches and parameters (e.g.,l_inferno,l_trif_fire,z_burn_max, combustion completeness bounds, and new flammability tunables) out of other modules/namelists. - Added a new PFT parameter
fireveg_c_to_atmos(_io)and updated TRIFFID to use it for fire carbon-to-atmosphere partitioning.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 89 changed files in this pull request and generated 2 comments.
Suppressed comments (7)
src/control/shared/jules_inferno_mod.F90:118
errorstatusis not initialised before being passed toereportin most branches (it is only assigned whenignition_methodis invalid). This can cause inconsistent or silent error handling depending on the compiler/runtime state.
src/control/shared/jules_inferno_mod.F90:129flam_sm_funcis used as a selector incalc_flam, but the namelist checker does not validate it. If a user sets an unexpected value,calc_flamcan use an uninitialisedf_sm_l.
src/science/fire/inferno/inferno_mod.F90:252l_cf_old_infernois hard-coded to.TRUE., which means the newjules_infernonamelist parameters (flam_*) are effectively ignored (relative humidity bounds andflam_rain_constare overridden). This defeats the purpose of extracting these parameters for tuning.
src/science/fire/inferno/inferno_mod.F90:295- The new rainfall scaling branch applies
EXP(-flam_rain_const * rain_l), butcheck_jules_infernorequiresflam_rain_const <= 0.0. With the leading minus this makes rainfall increase flammability and can overflow. The exponent should be consistent with the sign convention and should use a clearly defined rain unit.
src/science/fire/inferno/inferno_io_mod.F90:320 - Fuel normalisation is still hard-coded to 0.02/0.2, so the extracted
flam_fuel_low/flam_fuel_uptunables are not actually used.
rose-meta/jules-standalone/versions.py:99 - The upgrade macro sets
flam_rhum_low/flam_rhum_upto 0.1/0.9, but the model computes relative humidity in percent (0–100) andcheck_jules_infernoexpects 0–100. This would drastically change behaviour for upgraded apps.
self.add_setting(config, ["namelist:jules_inferno", "flam_rhum_low"], "0.1")
self.add_setting(config, ["namelist:jules_inferno", "flam_rhum_up"], "0.9")
doc/source/namelists/fire.nml.rst:169
- The literal word "buggy" in the namelist documentation looks like a placeholder and will ship to users.
buggy
e05dcd0 to
ade67dc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 34 changed files in this pull request and generated 8 comments.
Suppressed comments (2)
src/control/shared/jules_inferno_mod.F90:479
- The standalone namelist-open failure message duplicates the same phrase twice, which makes the error noisy and harder to read.
IF ( ERROR /= 0 ) &
CALL log_fatal("init_inferno", "Error opening namelist file fire.nml " // &
"Error opening namelist file fire.nml " // &
"(IOSTAT=" // TRIM(to_string(ERROR)) // " IOMSG=" // &
TRIM(iomessage) // ")")
rose-meta/jules-shared/jules-inferno/HEAD/rose-meta.conf:63
flam_sm_lowis described as a fraction of saturation (0–1) in code/docs, but rose metadata allows up to 10.0 here. This inconsistency can lead to invalid configurations being accepted by rose but rejected at runtime.
url=https://metoffice.github.io/jules/latest/namelists/jules_inferno.nml.html#JULES_INFERNO::flam_rain_const
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed functional issues (inverted z_burn_max missing-value check, calc_flam not correctly applying the new rainfall scaling parameter, and rose-stem JSON pointing at a non-existent source path) that would break configurations/builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
src/initialisation/standalone/init_inferno_mod.F90:39
- init_inferno_mod redefines ignition_method option constants locally even though they already exist in jules_inferno_mod. Duplicating these values risks drift if they ever change; import the parameters from jules_inferno_mod instead.
src/science/fire/inferno/inferno_mod.F90:239
- calc_flam still hardcodes the rainfall scaling via
cr=-2.0*s_in_day, which makes the newflam_rain_constargument effectively unused and keeps the historical double conversion behaviour. Removingcrhere avoids having an unused/duplicated constant and ensures rainfall scaling is controlled via the namelist parameter.
doc/source/namelists/fire.nml.rst:201 - Typo in documentation: "completness" should be "completeness".
Maximum DPM soil carbon pool combustion completness fraction.
doc/source/namelists/fire.nml.rst:208
- Typo in documentation: "completness" should be "completeness".
Minimum RPM soil carbon pool combustion completness fraction.
src/science/fire/inferno/inferno_mod.F90:284
flam_rain_constis passed into calc_flam but is not applied in the flammability rainfall term; instead a hardwired constant is used after converting rain to mm/day. This prevents tuning/removing the rainfall dependence via the new namelist and is inconsistent with the new interface.
- Files reviewed: 33/35 changed files
- Comments generated: 4
- Review effort level: Lite
github-actions
Bot
removed
the
cla-modified
Maggie (maggiehendry)
left a comment
Maggie (maggiehendry)
left a comment
There was a problem hiding this comment.
There's a build warning in the release notes as a result of the namelist change. The link no longer exists.
doc/source/release_notes/JULES7-6.rst:26: WARNING: nml:mem reference target not found: JULES_SOIL_BIOGEOCHEM::z_burn_max [ref.mem]
It's usual just to remove the link to keep the backwards compatability rather than repair the link.
- * Add a burn depth parameter :nml:mem:`JULES_SOIL_BIOGEOCHEM::z_burn_max` for when layered soil carbon is used so that fire only consumes soil carbon from above the burn depth. (#1514)
+ * Add a burn depth parameter ``JULES_SOIL_BIOGEOCHEM::z_burn_max`` for when layered soil carbon is used so that fire only consumes soil carbon from above the burn depth. (#1514)
There was a problem hiding this comment.
vn82_t141, vn82_t155 and vn82_t140 upgrade macros have been deleted.
There was a problem hiding this comment.
Still not quite right and after discussion with Eleanor Burke (@eleanorgb) and time constraints I'll either create a bob and retest or commit the fixes back to this branch.
There was a problem hiding this comment.
See last commit of maggiehendry-61-extracting-inferno-hardwired-flammability-params for a change to the upgrade macro. It is also the source file that doesn't exist in the Rivers-standalone. I've used lsm_id instead to indicate Rivers standalone instead of the presence of npft and reordered it so they are both together. jules_inferno still needs to be added as otherwise it gets user ignored in the Rivers-standalone Rose stem tests. This really needs sorting properly by extending the jules-shared framework to the JULES IO namelists to create a rivers-standalone metadata file. Until then though it's a case of ensuring the Rose stem tests aren't broken and the upgrade macro can be run on the coupled_rivers app in the LFric apps repository.
| CALL ereport(RoutineName, errorstatus, & | ||
| "z_burn_max must be positive & less than 10 meters") | ||
| END IF | ||
| IF ( ( l_layeredc .AND. l_trif_fire ) .OR. & |
Co-authored-by: Maggie <145924708+maggiehendry@users.noreply.github.com>
Pierre Siddall (Pierre-siddall)
left a comment
Pierre Siddall (Pierre-siddall)
left a comment
There was a problem hiding this comment.
Hi Eleanor Burke (@eleanorgb), thanks for this change. I only have one small suggestion on this review to improve the control flow of the program by reducing verbosity. If I've missed any context please feel free to let me know other wise, then I'll be happy to approve this and put it through testing.
This is fine. Ive committed. Co-authored-by: Pierre Siddall <43399998+Pierre-siddall@users.noreply.github.com>
ive made this change Pierre Siddall (@Pierre-siddall) |
|
NB This PR will need an LFRic apps PR to add the new |
|
Pierre Siddall (@Pierre-siddall) can I add you as code reviewer please for the linked MetOffice/lfric_apps#819 that adds the new jules_inferno_mod added with this PR. I'm just running the LFRic apps Rose stem test so it is still a draft at the moment. |
There was a problem hiding this comment.
Will be approved once the changed in the referenced bob are included. Please be aware of the linked MetOffice/lfric_apps#819 required.
There's also an amendment to jules-lfric required in the bob bd6785f:
> rose metadata-check -C rose-meta/jules-lfric/HEAD/
[V] rose.metadata_check.MetadataChecker: issues: 1
namelist:jules_triffid=fireveg_c_to_atmos_io=None=None
No metadata entry found
There was a problem hiding this comment.
See last commit of maggiehendry-61-extracting-inferno-hardwired-flammability-params for a change to the upgrade macro. It is also the source file that doesn't exist in the Rivers-standalone. I've used lsm_id instead to indicate Rivers standalone instead of the presence of npft and reordered it so they are both together. jules_inferno still needs to be added as otherwise it gets user ignored in the Rivers-standalone Rose stem tests. This really needs sorting properly by extending the jules-shared framework to the JULES IO namelists to create a rivers-standalone metadata file. Until then though it's a case of ensuring the Rose stem tests aren't broken and the upgrade macro can be run on the coupled_rivers app in the LFric apps repository.
| ! Check a suitable flam_sm_func was given | ||
| IF (flam_sm_func /= flam_sm_func_linear .OR. & | ||
| flam_sm_func /= flam_sm_func_exponential) THEN | ||
| CALL ereport( TRIM(RoutineName), errorstatus, 'flam_sm_func must be 1 or 2') | ||
| END IF |
There was a problem hiding this comment.
| ! Check a suitable flam_sm_func was given | |
| IF (flam_sm_func /= flam_sm_func_linear .OR. & | |
| flam_sm_func /= flam_sm_func_exponential) THEN | |
| CALL ereport( TRIM(RoutineName), errorstatus, 'flam_sm_func must be 1 or 2') | |
| END IF | |
| ! Check a suitable flam_sm_func was given | |
| IF ( flam_sm_func /= flam_sm_func_linear .AND. & | |
| flam_sm_func /= flam_sm_func_exponential ) THEN | |
| CALL ereport( TRIM(RoutineName), errorstatus, & | |
| 'flam_sm_func must be 1 or 2') | |
| END IF |
The logic here is now broken and fails the Rose stem tests. maggiehendry-61-extracting-inferno-hardwired-flammability-params contains the above suggested change and trac.log will be included on the PR summary.
|
Pierre Siddall (@Pierre-siddall) linked MetOffice/lfric_apps#819 is now ready for review. |
|
Oops - I re requested a review from Chantelle when she has already approved. I hope this isnt a problem - Im just waiting for Maggie to approve which she will hopefully do shortly, then I think all is OK? |
Maggie (maggiehendry)
added
Linked Apps
Hi Maggie (@maggiehendry), I'm happy to take a look sorry for not seeing this earlier. |
Thanks Eleanor Burke (@eleanorgb) for taking a look at the feedback, I'm now happy to approve this :) . |
Pierre Siddall (Pierre-siddall)
left a comment
Pierre Siddall (Pierre-siddall)
left a comment
There was a problem hiding this comment.
This now looks good to head into testing to me. Approved.
PR Summary
issue: [https://github.com//issues/61]
[https://github.com/MetOffice/um/pull/131]
test branch: https://github.com/eleanorgb/jules/tree/test_61-extracting-inferno-hardwired-flammability-params
Sci/Tech Reviewer: chantelleburton
Code Reviewer: Pierre Siddall (@Pierre-siddall)
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
Test Suite Results - jules - inferno/run1
Suite Information
Task Information
✅ succeeded tasks - 676
Test Suite Results - jules - jules-test-maggiehendry-61-extracting-inferno-hardwired-flammability-params/run1
Suite Information
Task Information
✅ succeeded tasks - 676
Security Considerations
Performance Impact
AI Assistance and Attribution
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