61 extracting inferno hardwired flammability params - #145
Eleanor Burke (eleanorgb) wants to merge 20 commits into
Conversation
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
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed functional issues (e.g., flam_rain_const not being applied in calc_flam, and Rose-stem JSON referencing a non-existent fire module path) that would prevent correct behavior and/or successful 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.
rose-meta/jules-shared/jules-inferno/HEAD/rose-meta.conf:44
- The Rose metadata range for
flam_fuel_lowhas no upper bound, butcheck_jules_infernoenforcesflam_fuel_up <= 1.0and the code treats fuel as a 0–1 normalised quantity. The metadata range should match the implemented constraints to prevent users configuring values that will always error at runtime.
This issue also appears on line 49 of the same file.
src/science/fire/inferno/inferno_mod.F90:284
flam_rain_constis passed intocalc_flambut never used; rainfall scaling still uses the hard-codedcrand a convertedrain_rate. This means the newjules_infernonamelist cannot tune the rainfall–flammability relationship as intended (and the legacy/double-conversion behavior described in the docs cannot be reproduced viaflam_rain_const).
src/science/fire/inferno/inferno_mod.F90:277- If
flam_sm_funcis ever set to an unexpected value,f_sm_lis left uninitialised before being used in theflam_lcalculation. Even thoughcheck_jules_infernovalidates values whenl_inferno=T, guarding here avoids undefined behavior if this routine is called before checks or from another code path.
src/control/shared/jules_inferno_mod.F90:184 flam_fuel_upis allowed to equalflam_fuel_lowhere, butinferno_io_moddivides by(flam_fuel_up - flam_fuel_low)when normalising fuel, which would be a divide-by-zero. Make the check strict (<=) to prevent invalid configurations.
IF ( ABS(flam_fuel_up - rmdi) < EPSILON(rmdi) ) THEN
CALL ereport( TRIM(RoutineName), errorstatus, &
"flam_fuel_up needs to be specified.")
ELSE IF ( flam_fuel_up < flam_fuel_low ) THEN
CALL ereport( TRIM(RoutineName), errorstatus, &
"flam_fuel_up must be > flam_fuel_low")
rose-meta/jules-shared/jules-inferno/HEAD/rose-meta.conf:55
- The Rose metadata range for
flam_fuel_uphas no upper bound, butcheck_jules_infernoenforcesflam_fuel_up <= 1.0and the code treats fuel as a 0–1 normalised quantity. The metadata range should match the implemented constraints to prevent users configuring values that will always error at runtime.
[namelist:jules_inferno=flam_fuel_up]
compulsory=true
description=Upper fuel load threshold for flammability
range=0.0:
sort-key=Panel-J13
type=real
url=https://metoffice.github.io/jules/latest/namelists/jules_inferno.nml.html#JULES_INFERNO::flam_fuel_up
- Files reviewed: 33/35 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
INFERNO flammability rainfall scaling is still hard-coded (ignoring the new namelist parameter) and there are a few validation/doc inconsistencies that can lead to incorrect behaviour or confusion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (4) — in code that hasn't changed since the last review.
src/science/fire/inferno/inferno_mod.F90:276
f_sm_lis not assigned ifflam_sm_funcis not 1 or 2, but it is still used in the flammability calculation; this can lead to undefined results if the routine is ever called with an unexpected value.
src/control/shared/jules_inferno_mod.F90:148- The error message for
flam_rhum_upsays it must be ">= flam_rhum_low", but the code rejects equality (<=) and equality would also cause a divide-by-zero incalc_flam. The message should match the actual constraint (strictly greater).
doc/source/namelists/fire.nml.rst:70 - The
JULES_INFERNOsection is missing a.. nml:namelist:: JULES_INFERNOdirective, and it referencesl_trif_fire/l_infernoas members ofJULES_INFERNO, but those switches are in theJULES_VEGETATIONnamelist.
src/control/shared/jules_inferno_mod.F90:201 - The
ccdpm_maxbounds check allows a value of exactly 1.0 (> 1.0is rejected), but the error message says "must be < 1.0". This is misleading for users.
This issue also appears on line 217 of the same file.
src/science/fire/inferno/inferno_mod.F90:284
flam_rain_constis passed intocalc_flambut not used; the rainfall term still uses the hard-coded constantcr, so changing the namelist parameter has no effect (and the upgrade macro/doc value will be ignored).
src/control/shared/jules_inferno_mod.F90:167- The
flam_fuel_upvalidation allowsflam_fuel_up == flam_fuel_low, butinferno_io_moddivides by(flam_fuel_up - flam_fuel_low); equality will cause a divide-by-zero.
ELSE IF ( flam_fuel_up < flam_fuel_low ) THEN
CALL ereport( TRIM(RoutineName), errorstatus, &
"flam_fuel_up must be > flam_fuel_low")
src/control/shared/jules_inferno_mod.F90:220
- The
ccrpm_maxbounds check allows a value of exactly 1.0 (> 1.0is rejected), but the error message says "must be < 1.0". This is misleading for users.
ELSE IF ( ccrpm_max > 1.0 ) THEN
CALL ereport( TRIM(RoutineName), errorstatus, &
"ccrpm_max must be < 1.0.")
END IF
src/control/shared/jules_inferno_mod.F90:55
- Typo in the
ccrpm_mincomment: "urn fraction" should be "burn fraction".
ccrpm_min = rmdi, &
! Minimum resistant plant material urn fraction (0 <= fraction <= 1)
ccrpm_max = rmdi, &
- Files reviewed: 25/27 changed files
- Comments generated: 2
- Review effort level: Lite
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
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