Repository navigation
Add minor reservoirs to hydrology - #143
Katie Blackford (KatieBlackford) wants to merge 50 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
|
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.
| .. 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).
There was a problem hiding this comment.
Initialising to zero in this way follows the general pattern that is used for river-related variables (which in general do not have the full flexibility that land-point variables have in terms of ways they can be initialised; from memory the only options are to read from a dump file or be set to zero).
| 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.
There was a problem hiding this comment.
Suggested changes have been applied in latest commit (97312c9)
| ! 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.
There was a problem hiding this comment.
James Bruten (@james-bruten-mo). This isn't called unless l_minor_reservoirs = T and currently the metadata makes l_minor_reservoirs unavailable when the parent model is the OASIS coupler when l_oasis_rivers = T. The code when l_oasis_rivers, abstracted_minor_res_rp is allocated with size 1, but the loop is i = 1, np_rivers with abstracted_minor_res_rp(i) which will currently go out of bounds if it ever does get called in the future. What is best to do here? I'm questioning the assumed vs explicit shape here as it'll be out-of-bounds regardless. If it isn't called does it matter if abstracted_minor_res_rp has a different size to what is allocated? An parallel ereport should be added to src/control/standalone/check_unavailable_options_mod.F90 for the l_oasis_rivers and l_minor_reservoirs combinations, which I'll request.
|
|
||
| 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.
There was a problem hiding this comment.
Changed to net_abstracted_river_rp(:) in latest commit (97312c9)
Co-authored-by: Maggie <145924708+maggiehendry@users.noreply.github.com>
…minor_res_global and tot_net_abstracted_river_global
|
Apologies. I haven't looked through your review changes yet. I have managed to do the UM & LFRic apps testing which didn't highlight any memory issues. However, there are a few bugs which I've fixed in a bob in order to run the UM & LFRic apps testing (see d874fb6). These will need adding to your branch. Please see the dr hook issues described in https://github.com/MetOffice/um/pull/121#pullrequestreview-5625351448 which will also need fixing. |
These problems should now have been addressed in the latest 2 commits. |
Maggie (maggiehendry)
left a comment
Maggie (maggiehendry)
left a comment
There was a problem hiding this comment.
A few minor items plus questions for James Bruten (@james-bruten-mo) (CR). Still to review the I/O (dump, initial conditions etc.) and will start another review for this for expediency.
| | | Only available if :nml:mem:`JULES_RIVERS::l_rivers` = TRUE and | | | ||
| | | :nml:mem:`JULES_RIVERS::l_minor_reservoirs` = TRUE. | | | ||
| +-------------------------------+-----------------------------------------------------------------------------------------------+------------+ | ||
| | ``minor_res_frac`` | Catchment area of minor reservoirs as fraction of area of gridbox. | np_rivers | |
There was a problem hiding this comment.
| | ``minor_res_frac`` | Catchment area of minor reservoirs as fraction of area of gridbox. | np_rivers | | |
| | ``minor_res_frac`` | Catchment area of minor reservoirs as fraction of area of gridbox. | np_rivers | |
Sorry, there's too many spaces now. Doc doesn't build with:
doc/source/output-variables.rst:420: ERROR: Malformed table.
There was a problem hiding this comment.
Fixed in commit af24399.
There was a problem hiding this comment.
Fixed in commit af24399.
| ! Array arguments with INTENT(IN) | ||
| !------------------------------------------------------------------------------ | ||
| REAL(KIND=real_jlslsm), INTENT(IN) :: & | ||
| abstracted_minor_res_rp(np_rivers), & |
There was a problem hiding this comment.
James Bruten (@james-bruten-mo). This isn't called unless l_minor_reservoirs = T and currently the metadata makes l_minor_reservoirs unavailable when the parent model is the OASIS coupler when l_oasis_rivers = T. The code when l_oasis_rivers, abstracted_minor_res_rp is allocated with size 1, but the loop is i = 1, np_rivers with abstracted_minor_res_rp(i) which will currently go out of bounds if it ever does get called in the future. What is best to do here? I'm questioning the assumed vs explicit shape here as it'll be out-of-bounds regardless. If it isn't called does it matter if abstracted_minor_res_rp has a different size to what is allocated? An parallel ereport should be added to src/control/standalone/check_unavailable_options_mod.F90 for the l_oasis_rivers and l_minor_reservoirs combinations, which I'll request.
| * minor_res_frac(i) * rivers_boxareas_rp(i) * dt | ||
| surf_runoff(i) = surf_runoff(i) * ( 1.0 - minor_res_frac(i) ) | ||
| ! Remove water abstracted. | ||
| minor_res_storage(i) = minor_res_storage(i) - abstracted_minor_res_rp(i) |
There was a problem hiding this comment.
Can you add an ereport to src/control/standalone/check_unavailable_options_mod.F90 for l_oasis_rivers and l_minor_reservoirs (and similarly for l_water_resources) as I don't think there's anything in the code that'll prevent this scenario (jules_water_resources currently isn't read by Rivers-standalone, but for completeness please add so it's obvious they can't be used together). Perhaps mention in the message that relevant arrays would need allocating at full size. The metadata already makes this combination unavailable.
There was a problem hiding this comment.
I have added two new ereport to src/control/standalone/check_unavailable_options_mod.F90 in commit af24399.
| REAL(KIND=real_jlslsm), INTENT(IN) :: & | ||
| ! arrays with intent(in) | ||
| net_abstracted_river_rp(np_rivers), & | ||
| net_abstracted_river_rp(:), & |
There was a problem hiding this comment.
James Bruten (@james-bruten-mo) can we have your expert advice on this please. When this routine is called when l_oasis_rivers=T (Rivers-standalone coupled via OASIS) net_abstracted_river_rp has size 1, otherwise it has size np_rivers. There's a loop i = 1, np_rivers with net_abstracted_river_rp(i) but the loop is only entered when sw_river_source > 0, which will only be the case if l_water_resources=T. This currently cannot happen as l_water_resources=F for Rivers-standalone as init_jules_water_resources is currently not called so jules_water_resources isn't read. Does it matter what size of array is passed? I had originally suggested it would be better passing as assumed size as a result of the mismatch in size. I'm now wondering. Similarly for RFM routine. Also see comment regarding src/science/river_routing/route_minor_reservoirs_mod.F90.
There was a problem hiding this comment.
Just to say that it would be great to have some sort of standard advice (documented) as to how to proceed with this kind of issue (e.g. we have had previous email discussions - which I can't currently find - around what to do with assumed-size arrays and the likes). But that (wider) advice can wait for now!
Is there any danger if we declare as assumed size here? (I prefer to avoid assumed size where possible, but where the size depends on the configuration as here they seem like the possible answer.) As Maggie says, the IF clauses should protect it from ever being run, and I (possibly naively) feel that assumed size would avoid any memory access issues when it is size=1.
There was a problem hiding this comment.
I'm not really sure. I imagine it doesn't make much difference either way given the if statements. I think I'd edge towards assumed size here, given its configuration dependent.
Maggie (maggiehendry)
left a comment
Maggie (maggiehendry)
left a comment
There was a problem hiding this comment.
I've now had a look at the dump IO and all looks fine 😄. Just comments, no action required.
- I'd strongly encourage you to add a Rose stem test otherwise your work may be broken by someone else.
- To make you aware, I will be having a look at the design of the initialisation of the Rivers required vars soon as LFRic coupled requires some additional functionality.
- In general, need to consider whether the water resources code should be made available to Rivers-standalone. Obviously not one for now but something to consider. I've mentioned this PR to Dan Copsey (@DanCopsey) in an email.
| ! Description: | ||
| ! Allocate river point variables, initialise, and associate pointers. | ||
| ! Also allocates at least one variable on land points. | ||
| ! Also allocates related variables on land points. |
There was a problem hiding this comment.
I'm not convinced that this is the best place to allocate land point arrays, but as I need to sort out all the array allocation anyway I'm not going to think overly about it now.
drhook_check now passes in UM Rose stem 😃 |
Agreed re rose stem - this is already on my "tracker" issue #183 . Re rivers-standalone - just to note that in "true standalone mode" (i.e. just a river model, not coupled to anything) there probably is no (or very limited) use for the water resource (WR) code as most bits require some sort of input from the land code (e.g. to calculate abstractions from reservoirs). In coupled mode (e.g. in LFRic) the WR functionality is potentially useful - though whether it has to be contemplated any time soon is another question. (Not sure that helps you at all here, but I just thought I would attempt to clarify!) |
Thanks for the heads up about the minor reservoirs and water resources code. I think our plan for the LFRic coupled climate model is to eventually move to using Camaflood so we may have no need for this but it looks good though. |
…asis_rivers and l_minor_reservoirs / l_water_resources to check_unavailable_options_mod.
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/run2
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