Repository navigation
Add minor reservoirs to hydrology - #143
Katie Blackford (KatieBlackford) wants to merge 43 commits into
Conversation
Co-authored-by: James Bruten <109733895+james-bruten-mo@users.noreply.github.com>
…h vn8.0_minor_reservoirs).
….1_minor_reservoirs
…de to rivers code) as a variable on "global" land points rather than scattering and then regathering.
…(not local) land point variable.
return_flow_sw now called return_flow_river for clarity.
Updates from config-dump and metadata-check.
…ble, before a later decision based on configuration. Makes coupling slightly more clear/clean. Also tidying and adding DrHook (right now I'm bemused as to how some bits were compilable previously).
…to vn8.2_minor_reservoirs
github-actions
Bot
added
the
cla-signed
Douglas Clark (doucla)
added
the
Linked UM
| ! Units | ||
| "kg m-2 s-1" & | ||
| ) / | ||
| !----------------------------------------------------------------------------- |
There was a problem hiding this comment.
conveyance loss added here but not used directly in the minor reservoirs calculation
There was a problem hiding this comment.
We added two diagnostics (conveyance_loss and river_abstracted) that are not directly related to minor reservoirs because we found ourselves looking for this extra information when trying to assess how well the model is working. As the representation of water resources is becoming more complicated, so we need further diagnostics to track what is happening (and to provide the information that a science user might require).
There was a problem hiding this comment.
Thanks. It may be worth clarifying that this relates only to the water resource system.
|
This PR requires an LFRic apps change: |
There was a problem hiding this comment.
I've just had a quick scan through for now and have noted the following:
- The lfric_apps build fails as a result of the change to
jules_rivers_allocandwater_resources_allocarguments. The newjules_rivers_allocarguments don't currently do anything so I'd be inclined to remove them for now until they are needed. However, the change to the arguments ofwater_resources_allocwill need an LFRic apps branch. - New ancillary and dump fields are added, but there is no equivalent added to the UM. If it cannot be used in the UM it should be made unavailable by adding it to the
l_jules_parenttrigger list and thecheck_unavailable_optionsroutine for the UM. The only trigger I can see is the one forl_riversrather thani_river_vnalthough the UM can now use RFM as well as UM_TRIP as far as I recall. - As well as shared code changes, there are code changes which affect OASIS-Rivers. Therefore, the UM and the LFRic apps Rose stem tests need to be run and please ensure that includes lfric_coupled_developer.
Can you please have a look at these before I proceed with my review.
FYI I've just opened MetOffice/lfric_apps/issues/812 to look into the possibility of moving the jules_*_alloc calls in LFRic apps into a routine in the JULES repo as it's all too common that the argument list change gets missed. However, it's important to note that when any shared code is developed, the LFRic apps & UM Rose stem tests both need to be run to pick up things like this.
Please enter the commit message for your changes. Lines starting
| ! Remove net abstraction from rivers from surface runoff - in effect the | ||
| ! surface runoff variable becomes a more generic source/sink term for rivers. | ||
| ! The resulting term can be negative. | ||
| !------------------------------------------------------------------------------ |
There was a problem hiding this comment.
Noting that the net_abstracted_river_rp term will be weighted according to the availability of water in the rivers or groundwater etc. Any negative values in rivers%rrun_surf_rp will be small (how small?) and will affect the conservation of water.
Sonja (sfolwell)
left a comment
Sonja (sfolwell)
left a comment
There was a problem hiding this comment.
SciTech Review. I approve this code change. The change introduces minor reservoirs as a bulk storage term for a user defined fraction of surface runoff which is then available to meet anthropogenic water demands. It operates as described in the documentation https://github.com/user-attachments/files/31918608/Water_resources_technical_doc_260907.pdf
|
Katie Blackford (@KatieBlackford), Douglas Clark (@doucla) I was just wondering if you'd seen what I'd noted in relation to LFRic apps? If you need me to run the lfric_coupled_developer testing let me know. |
Maggie - yes, thanks, we saw your comments relating to LFRic apps. We have made various tweaks recently but are also investigating some unexpected (bad!) behaviour that we found in more recent tests - we're trying to understand what's going on before hopefully completing this work/PR. |
…is done by no longer cominging the abstraction with the surface runoff field, but instead keeping abstractions separate. net_abstracted_river_rp is now passed into SUBROUTINE rivers_route_rp (now with units of kg rather than kg m-2). Note that the treatment of negative runoff in RFM has now reverted to how it looked at approx vn7.9, BEFORE we started to incorporate abstractions.
|
Sonja (@sfolwell) - we have had to make a few late changes on this branch since your review, primarily to stop the occurrence of negative river stores. I think the changes Oct 12-14 are most relevant here. Broadly, although the abstraction from rivers was calculated to avoid taking more water than is available (at the start of the timestep), if we look at the TRIP code (rivers_route_trip_mod.F90) the store was also being reduced by the outflow over the timestep - and the sum of those (abstraction and outflow) could result in store < 0. (Remember that the abstraction term was being combined with the surface runoff, for "convenience" [so we thought].) The solution has been to keep the abstraction separate from surface runoff, and to remove the abstraction as a separate action before any other river flow calculations are done. This has removed the problem of negative stores, without affecting the positive results seen in all other aspects of our testing. I'm not suggesting that you need to redo your review! But perhaps you could take a look and indicate your continued approval via a comment. Thanks! |
Douglas Clark (doucla)
added
the
Linked Apps
Maggie (maggiehendry)
left a comment
Maggie (maggiehendry)
left a comment
There was a problem hiding this comment.
There's a possible issue with memory corruption when l_oasis_rivers. I tried to test this with the LFRic apps branch, but they are at incompatible revisions. To be compatible this branch needs to be upgraded to at least 332ca85. Please see MetOffice/lfric_apps#859 (comment). This branch also needs to be tested with the JULES group in the UM Rose stem tests. The UM branch is also not at a compatible revision (see https://github.com/MetOffice/um/pull/121#issuecomment-6273619299).
I haven't finished reviewing this as I haven't had a chance yet to look at the ancillaries, dump & model interface changes. I am most likely out of the office until Thursday so I thought I'd submit this now and start a new review on Thursday.
| | ``minor_res_capacity_grid``| Storage capacity of minor reservoirs (kg). | | ||
| | | | | ||
| | | This is only used if :nml:mem:`JULES_RIVERS::l_minor_reservoirs` = TRUE. | | ||
| | | | | ||
| +----------------------------+-----------------------------------------------------------------------------------------------------------+ | ||
| | ``minor_res_frac_grid`` | Catchment area of minor reservoirs as fraction of area of gridbox. | | ||
| | | | | ||
| | | This is only used if :nml:mem:`JULES_RIVERS::l_minor_reservoirs` = TRUE. | | ||
| | | | | ||
| +----------------------------+-----------------------------------------------------------------------------------------------------------+ |
There was a problem hiding this comment.
These need to be added to the values list under [namelist:jules_rivers_props=var] with a fail-if for l_minor_reservoirs=.true. and not any(this)=minor_res_frac_grid or not any(this)=minor_res_capacity_grid. You'll need to test the logic to make sure it's correct.
| | Required if :nml:mem:`JULES_RIVERS::l_rivers` = TRUE, :nml:mem:`JULES_RIVERS::l_minor_reservoirs` = TRUE and | | ||
| | :nml:mem:`JULES_INITIAL::dump_file` = TRUE | | ||
| +----------------------------------+-----------------------------------------------------------------------------------------+---------+ | ||
| | ``minor_res_storage`` | Water stored in minor reservoirs, on river routing points (kg). | none | |
There was a problem hiding this comment.
Can you add this after inland_outflow as this and rivers_outflow_rp are both OASIS coupling fields. It makes more sense to keep these together.
| .. warning:: | ||
| if :nml:mem:`JULES_RIVERS::l_rivers` = TRUE, :nml:mem:`JULES_RIVERS::l_minor_reservoirs` = TRUE and :nml:mem:`JULES_INITIAL::dump_file` = FALSE, | ||
| ``minor_res_storage`` is initialised to zero. |
There was a problem hiding this comment.
I need to check that this is actually the case. It might be missing data.
There was a problem hiding this comment.
minor_res_storage is initialised to zero (see jules_rivers_props_mod).
| | Variables that quantify how much water is available and abstracted: | | ||
| +---------------------------+--------------------------------------------------------------------------------+------------+ | ||
| | ``gw_avail`` | Groundwater that is available for abstraction at start of timestep (kg) . | | | ||
| | | Only available if groundwater sources are modelled. | | |
There was a problem hiding this comment.
Is there a switch connected to ground water sources that can be referenced?
| errorstatus = 10 | ||
| CALL ereport( RoutineName, errorstatus, & | ||
| "Error related to allocation of runoff variables." ) | ||
| "Error related to allocation of abstraction variables." ) |
There was a problem hiding this comment.
This error message is not just for the abstraction variables. The error message is misleading. If you'd like a separate message for the abstraction variables, can you reset the error sum after the runoff variables error message please.
| REAL(KIND=real_jlslsm), INTENT(IN) :: & | ||
| abstracted_minor_res_rp(np_rivers), & | ||
| ! Water abstracted from minor reservoirs over river timestep, on river | ||
| ! points (kg). | ||
| net_abstracted_river_rp(np_rivers) | ||
| ! Water abstracted from rivers over river timestep, on river points (kg). |
There was a problem hiding this comment.
| REAL(KIND=real_jlslsm), INTENT(IN) :: & | |
| abstracted_minor_res_rp(np_rivers), & | |
| ! Water abstracted from minor reservoirs over river timestep, on river | |
| ! points (kg). | |
| net_abstracted_river_rp(np_rivers) | |
| ! Water abstracted from rivers over river timestep, on river points (kg). | |
| REAL(KIND=real_jlslsm), INTENT(IN) :: & | |
| abstracted_minor_res_rp(:), & | |
| ! Water abstracted from minor reservoirs over river timestep, on river | |
| ! points (kg). | |
| net_abstracted_river_rp(:) | |
| ! Water abstracted from rivers over river timestep, on river points (kg). |
When l_oasis_rivers these are allocated with a size of 1, not np_rivers. Passing an explicit shape array of the wrong size will end up in the memory being corrupted so just pass as assumed shape.
| ! Array arguments with INTENT(IN) | ||
| !------------------------------------------------------------------------------ | ||
| REAL(KIND=real_jlslsm), INTENT(IN) :: & | ||
| abstracted_minor_res_rp(np_rivers), & |
There was a problem hiding this comment.
This also is size 1 when l_oasis_rivers. Probably safer to pass all as assumed size. I can't test this as this branch is three revisions behind the associated LFRic apps branch. This branch will need updating to at least 332ca85 in order to test with the LFRic apps branch.
|
|
||
| REAL(KIND=real_jlslsm), INTENT(IN) :: & | ||
| ! arrays with intent(in) | ||
| net_abstracted_river_rp(np_rivers), & |
There was a problem hiding this comment.
See previous comments regarding allocated size.
Co-authored-by: Maggie <145924708+maggiehendry@users.noreply.github.com>
7 participants
PR Summary
The code allows for minor reservoirs, these are defined as small water bodies that are not on the main river stem but collect and retain local runoff which is then available for water abstractions.
Closes #32
Sci/Tech Reviewer: Sonja (@sfolwell)
Code Reviewer: James Bruten (@james-bruten-mo)
linked MetOffice/um#121
linked MetOffice/lfric_apps#859
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
trac.log
Test Suite Results - jules - vn8.2_minor_reservoirs_test/run4
Suite Information
Task Information
✅ succeeded tasks - 213
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