Bare soil evaporation separated over irrigated and non-irrigated soil -old as from stable pre Maggie's meta data changes #139 - #150
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Suppressed comments (3)
src/science/surface/physiol_jls_mod.F90:843
- This loop nests an inner
IF ( l_irrig_dmd )inside an outerIF ( l_irrig_dmd ), and repeatssthu_nir_soilt = sthu_soilttwice. This is redundant and makes the logic harder to follow; it can be simplified to a single conditional on frac_irr_soilt.
sthu_nir_soilt(l,m,k) = sthu_soilt(l,m,k)
IF ( l_irrig_dmd ) THEN
sthu_nir_soilt(l,m,k) = sthu_soilt(l,m,k)
IF ( frac_irr_soilt(l,m) < 1.0 ) THEN
sthu_nir_soilt(l,m,k) = &
rose-meta/jules-standalone/versions.py:51
- This upgrade macro still has template placeholders in the class name and docstring (vnYY_txxxx / "by Author"). Please rename the class to match the tag (and remove placeholder text) to be consistent with the existing rose upgrade macros.
class vnYY_txxxx(MacroUpgrade):
"""Upgrade macro from JULES by Author"""
src/science/surface/jules_land_sf_explicit_jls.F90:1256
- There is a commented-out argument ("not needed?") in the call argument list. Please remove it to avoid ambiguity about the actual calling interface.
gs_irr_surft, smc_irr_soilt, wt_ext_irr_surft, gc_irr_surft, &
gs_nir_surft, &
!! not needed? gc_nir_surft, &
| !$OMP PARALLEL IF(l_do_omp) DEFAULT(NONE) PRIVATE(l,m,n) SHARED(dzsoil, & | ||
| !$OMP fsoil_tot, land_pts, smc_irr_soilt, sthu_irr_soilt,nsoilt, & | ||
| !$OMP smvcst_soilt, gs_irr_surft,gc_irr_surft, nsurft,l_do_omp) | ||
| !$OMP fsoil_tot, land_pts, smc_irr_soilt, sthu_irr_soilt, nsoilt, & | ||
| !$OMP smc_nir_soilt, smc_soilt, sthu_nir_soilt, & | ||
| !$OMP smvcst_soilt, gs_irr_surft, gc_irr_surft, nsurft, l_do_omp, & | ||
| !$OMP fsoil_irr_tot, fsoil_nir_tot, frac_irr_soilt, & |
| !$OMP END DO NOWAIT | ||
| END IF | ||
| IF (l_soil_evap_irrig_expl) THEN | ||
| !$OMP DO SCHEDULE(STATIC) |
|
I am happy with the principle of adding this functionality into the model and have approved it from the point of view of the surface module. |
|
Hi Nic. I'm just doing my review. While I was doing it I noticed that the UM & LFRic apps Rose stem tests haven't been run. As shared code has been changed, these will need running. There's a small change to the UM |
Maggie (maggiehendry)
left a comment
There was a problem hiding this comment.
Hi Nic. As you've noted elsewhere in the PR I was in the process of deleting the metadata from um-atmos. This has now gone onto the trunk as of today. All the jules_irrig metadata has been moved to rose-meta/jules-shared/jules-irrig in the JULES repo and is now imported to rose-meta/jules-standalone and rose-meta/jules-um also in the JULES repo. I've noted that there is a small change needed to the UM jules_irrig namelist reading routine to pass the UM Rose stem tests. What I'd suggest is you upgrade your JULES branch to the head of main to pick up the metadata changes (see Updating Branches). Add your new metadata changes to jules-shared/jules-irrig/HEAD/rose-app.conf and fix the UM read_nml_jules_irrig before you run the UM Rose stem tests. You'll also need a UM upgrade macro to add the new switch. Unfortunately the upgrade macros can't be imported. I've also noticed that your PR says you wish to "merge 26 commits into MetOffice:stable instead of MetOffice:main. You might need to create a new PR, but @MetOffice/ssdteam can advise on that. I means though that you can't upgrade your branch via the "Web Browser" method as it isn't out-of-date with the base branch as the base branch is MetOffice:stable.
| :nml:mem:`nstep_irrig` = NINT(frequency of irrigation update (in sec)) / :nml:mem:`JULES_TIME::timestep_len` | ||
|
|
||
|
|
||
| .. nml:member:: l_soil_evap_irrig_expl |
There was a problem hiding this comment.
I wonder if there might be a clearer way to name this switch. I've been pondering this as explicit might mean more to someone other than being calculated from two different sources. I've mulled it over and came up with l_soil_evap_irrig_discrete, l_soil_evap_irrig_split, l_soil_evap_irrig_separate. What do you think? I'm not sure I'm keen on any of them, but thought I'd open the discussion as I'm not sure it's clear from the name what it's for.
There was a problem hiding this comment.
Agreed. Changing to l_soil_evap_irrig_separate
Hi Maggie Maggie (@maggiehendry) All are the following correct? Do I need to download the UM and open a UM ticket too? I'm slightly confused as this is in jules repository: |
PR Summary
<-- Bare soil evaporation separated over irrigated and non-irrigated soil #139 -->
Sci/Tech Reviewer: hkatty
Code Reviewer: James Bruten (@james-bruten-mo)
<-- #139 -->
<-- this allows the irrigated and non-irrigated bare soil evaporation terms to be calculated from the irrigated and non-irrigated soil moisture columns (l_soil_evap_irrig_expl=True) rather than the grid-box mean soil moisture (l_soil_evap_irrig_expl=False) as previously -->
<-- List any linked PRs here
None
-->
<-- List any blocking PRs or issues to be closed here
None
-->
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
<-- Other testing performed:
rose-stem test also applied to the test branch test_separate_irrigated_bare_soil_evap
individual point comparison with l_soil_evap_irrig_expl=T compared with stable.
See the issue for more details.
-->
trac.log
<-- Development branch:
Test Suite Results -
Test Suite Results - jules - vn8.2_separate_irrigated_bare_soil_evap/run3
Suite Information
Task Information
✅ succeeded tasks - 676
Test branch:
Test Suite Results - jules - vn8.2_test_separate_irrigated_bare_soil_evap/run3
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