Bare soil evaporation separated over irrigated and non-irrigated soil #139 - #150
Bare soil evaporation separated over irrigated and non-irrigated soil #139#150Nic Gedney (nicgedney) wants to merge 16 commits into
Conversation
github-actions
Bot
added
the
cla-required
|
Hello Nic Gedney (@nicgedney)! 👋 Thank you for your contribution. Since this is your first time contributing to this repository, we ask that you sign our Contributor Licence Agreement (CLA). To agree to the CLA, please add your details (GitHub username, Real Name, Affiliation, and Date) to the CONTRIBUTORS.md file (create one, if required) in the development branch for this PR. After signing the CLA, you won't need to do this again for future PRs. |
Nic Gedney (nicgedney)
changed the title
Nic Gedney (nicgedney)
assigned
Nic Gedney (nicgedney) and
hkatty and unassigned
hkatty and
Nic Gedney (nicgedney)
John Edwards (JMEdwardsXtr)
left a comment
John Edwards (JMEdwardsXtr)
left a comment
There was a problem hiding this comment.
As yet, I only partially understand the science here, but I have made such comments as I can. I would like the logic to be more apparent. I will be happy to look again once these comments have been addressed.
I'm not sure exactly what reviewing role I should be performing here. The PR says that hkatty will do the science review, but approval is also required from the owners of the relevant JULES modules, which approval I can give for the surface module.
| :type: logical | ||
| :default: F | ||
|
|
||
| Switch controlling whether the bare soil evaporation from the irrigated and non-irrigated part of the grid-box (or soil tile) is controlled by the mean soil moisture or the separate irrigated and non-irrigated soil moisture columns. |
There was a problem hiding this comment.
irrigated and non-irrigated part_s_
| No effect. | ||
|
|
||
| This must be set to FALSE if :nml:mem:`JULES_IRRIG::irrig_option` = 0. | ||
| This must be set to FALSE if :nml:mem:`JULES_IRRIG::irrig_option` = 2. |
There was a problem hiding this comment.
I'm probably going to seem foolish here, but I'm having trouble working out what is going on. My interpretation is that irrig_opt=0 means no irrigation, irrig_opt=1 will mean the current scheme that is currently represented by l_irrig_dmd=.T., and that irrig_opt=2 will be the new scheme. So this says that the option only works with the current scheme, but then I am confused by L2649 in rose-meta/jules-standalone/HEAD/rose-meta.conf where the option is triggered if l_irrig_dmd is .F.
As a cautionary query here, if it's meant to work with l_irrig_dmd=.T., can I check that no modification of src/science/soil/hydrol_jls_mod.F90 is needed because the extraction is already treated properly?
| set_irrfrac_on_irrtiles, nstep_irrig, irrig_option | ||
| set_irrfrac_on_irrtiles, nstep_irrig, irrig_option, & | ||
| l_soil_evap_irrig_expl | ||
|
|
There was a problem hiding this comment.
I would have expected the new variable to be added to my_nml in read_nml_jules_irrig starting on L339. It is worth adding any checks to check_jules_irrig on L289?
| * esoil_surft(l,n) | ||
|
|
||
| IF ( l_soil_evap_irrig_expl ) THEN | ||
| wt_ext_nir_surft(l,m,n) = wt_ext_surft(l,m,n) |
There was a problem hiding this comment.
You could use an IF...ELSE...END IF construct here, but I'm not sure whether that's faster than what you have on current machines or not.
| + tile_frac(l,n) & | ||
| * wt_ext_nir_surft(l,m,n) & | ||
| * esoil_nir_surft(l,n) & | ||
| * (1.0 - frac_irr_surft(l,n)) & |
There was a problem hiding this comment.
Presumably, if soil and surface tiles match frac_irr_surftt and frac_irr_soilt are the same, but it is wise to leave the fractions in place to make it clear what is happening, should someone later try to generalize the code.
| USE jules_surface_mod, ONLY: l_aggregate, l_epot_corr | ||
| USE jules_science_fixes_mod, ONLY: l_fix_moruses_roof_rad_coupling, & | ||
| l_fix_neg_snow | ||
| USE jules_irrig_mod, ONLY: l_irrig_dmd |
There was a problem hiding this comment.
I can't see where this logical is used in the file.
| END DO | ||
| !$OMP END DO NOWAIT | ||
| END IF | ||
| IF (l_soil_evap_irrig_expl) THEN |
There was a problem hiding this comment.
I haven't properly understood the relationship between this variable and l_irrig_dmd.. In sf_evap_jls we had a general block of code and another under l_irrig_dmd. Can I just check that everything is consistent here?
| q1_land(l) = qw_1(i,j) | ||
| cosz_gb(l) = cos_zenith_angle(i,j) | ||
| fsoil_tot(l) = frac(l,soil) | ||
| IF ( l_soil_evap_irrig_expl ) THEN |
There was a problem hiding this comment.
This may be a matter of algorithmic convenience, but why are we irrigating the bare-soil tile?
4 participants
PR Summary
<-- Bare soil evaporation separated over irrigated and non-irrigated soil #139 -->
Sci/Tech Reviewer: <-- hkatty -->
Code Reviewer:
<-- #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
-->
trac.log
<-- # Test Suite Results - jules - vn8.2_separate_irrigated_bare_soil_evap/run2
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