1033 bug in accumulated runoff

[vgro]

This PR fixes the water accumulation bug described in #1033 and reorganizes how the streamflow is calculated:

• there is no longer a link to the previous time step, only surface and subsurface flows from the current time step contribute to the stream flow.
• I renamed the accumulated flows to channel_inflow and the function accumulate_horizontal_flow route_horizontal_flow.
• I tried to explain all this better in the docs
• I added a different DEM to the implementation documentation to show the actual connections, but there is currently an issue with the mapping, see note below.
• I added a test to do a monthly water balance between precipitation and the stream flow in the lowest grid cells (if no lowest grid cell, the next lowest group of cells)

NOTE: there seems to be a problem with the mapping of the elevation on the grid, likely because I did not provide a projection, but need to keep an eye on, see docs built here.

Fixes #1033

Type of change

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist

• Make sure you've run the pre-commit checks: $ pre-commit run -a
• All tests pass: $ poetry run pytest

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works
• Relevant documentation reviewed and updated

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.67%. Comparing base (9ad8b0e) to head (8388d9f).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1043      +/-   ##
===========================================
+ Coverage    94.60%   94.67%   +0.06%     
===========================================
  Files           79       79              
  Lines         6937     6964      +27     
===========================================
+ Hits          6563     6593      +30     
+ Misses         374      371       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vgro]

I added this plot to show that the data changes orientation when it is mapped on the grid. Any suggestions what's going on?

[davidorme]

Oh man, we so shouldn't have used an NxN grid in the example data. I'll try and look into what is going on in the data loader.

[vgro]

created #1054

[sallymatson]

I think it looks good but I just have a few clarifications about some terminology / explanations.

Another question (not to fix in this PR necessarily) but with this idea of flow and the upstream cells - are upstream cells just ANY upstream cell in the grid? Because that would not accurately represent flow; e.g. if there is a lake basin then cells north of the river would not flow through cells south of the river, even if the elevation is higher. So is there some sense of directionality built into the flow maps?

I can/will also check that in the code but just wanted to raise it before it slips my mind.

[sallymatson]

Just to clarify. Shouldn't river discharge NOT be a sum of flow? In my (new) understanding, flow represents how much water flows through each cell in the timestem. Whereas the river discharge should only account for water that terminates in that cell. Is that correct?

So the sum of the flow across the whole grid will be greater than the sum of river discharge across the whole grid.

[vgro]

the river discharge does still include all the upstream cells, but not from the previous timestep, that was the bug. So the sum of river discharge will be larger than the rainfall-evaporation, but the local generation sum should be the same (over a longer period)

[sallymatson]

Can the names be a bit more standardized? The return variable is called channel_inflow but here you say the return is total streamflow; I actually think that total_streamflow is a bit more descriptive at least in how I understand what is going on

[sallymatson]

But in general would just prefer if this terminology somewhat mirrors the code/variables below, not as picky on which it is

[vgro]

I made an attempt to do this, please tell me if it is still not clear, this should be really easy and I'm happy to adjust more if needed

[vgro]

I think it looks good but I just have a few clarifications about some terminology / explanations.

Another question (not to fix in this PR necessarily) but with this idea of flow and the upstream cells - are upstream cells just ANY upstream cell in the grid? Because that would not accurately represent flow; e.g. if there is a lake basin then cells north of the river would not flow through cells south of the river, even if the elevation is higher. So is there some sense of directionality built into the flow maps?

I can/will also check that in the code but just wanted to raise it before it slips my mind.

Thank you so much for your comments! I made some changes to the docs and variables names, I hope it it now clearer what is what. We have the runoff and local generation in each grid cell, and we have the inflow which is everything coming from above, and we have the river discharge which is the sum of all. I also added the definitions to the glossary and linked to it in the documentation.

Regarding your question above, The way the drainage map is calculated should include direction, we first find the lowest neighbor for each grid cell, so where the water flows, and then identify upstream cells backwards.

[vgro]

@sallymatson I just read this definition in a paper which sounds very clear, I could adopt this (basically rename what is now 'total_river_discharge' to 'total_runoff':

'Based on that, hereinafter we refer to river discharge, Q, to indicate the amount of water passing a particular point of a river (in m3 s−1), whereas runoff, R, is regarded as the depth of water produced from a drainage area during a particular time interval (in mm). The difference between the two quantities is related to the routing processes that allow runoff to transform into river discharge.'

[lelavathy]

The terminology used to describe flow components should be standardized for consistency. For example, some sections refer to surface flow and subsurface flow, while others use surface runoff and subsurface runoff.

[lelavathy]

@sallymatson I just read this definition in a paper which sounds very clear, I could adopt this (basically rename what is now 'total_river_discharge' to 'total_runoff':

'Based on that, hereinafter we refer to river discharge, Q, to indicate the amount of water passing a particular point of a river (in m3 s−1), whereas runoff, R, is regarded as the depth of water produced from a drainage area during a particular time interval (in mm). The difference between the two quantities is related to the routing processes that allow runoff to transform into river discharge.'

[lelavathy]

@sallymatson I just read this definition in a paper which sounds very clear, I could adopt this (basically rename what is now 'total_river_discharge' to 'total_runoff':
'Based on that, hereinafter we refer to river discharge, Q, to indicate the amount of water passing a particular point of a river (in m3 s−1), whereas runoff, R, is regarded as the depth of water produced from a drainage area during a particular time interval (in mm). The difference between the two quantities is related to the routing processes that allow runoff to transform into river discharge.'

yes agree, total runoff (mm) is the right term water balance comparisons

[lelavathy]

If we follow Figure 9, subsurface runoff (Q2) and interflow (Q3) are currently not implemented. This means that: Qtotal= Q surface + Q sub baseflow (Q4) + Q baseflow (Q5) right?

[vgro]

this is now outdated, I changed it to:
Qtotal = Rsurface + Rsubsurface

[vgro]

Sorry I just made the changes as you are still reading it, I will wait for you to complete the review :-)

[lelavathy]

I’ve completed the review. I’m a bit confused with the some of the flow terminology. It's okay, we can discuss this in our meeting.

[vgro]

I think it looks good but I just have a few clarifications about some terminology / explanations.
Another question (not to fix in this PR necessarily) but with this idea of flow and the upstream cells - are upstream cells just ANY upstream cell in the grid? Because that would not accurately represent flow; e.g. if there is a lake basin then cells north of the river would not flow through cells south of the river, even if the elevation is higher. So is there some sense of directionality built into the flow maps?
I can/will also check that in the code but just wanted to raise it before it slips my mind.

Thank you so much for your comments! I made some changes to the docs and variables names, I hope it it now clearer what is what. We have the runoff and local generation in each grid cell, and we have the inflow which is everything coming from above, and we have the river discharge which is the sum of all. I also added the definitions to the glossary and linked to it in the documentation.

Regarding your question above, The way the drainage map is calculated should include direction, we first find the lowest neighbor for each grid cell, so where the water flows, and then identify upstream cells backwards.

I just printed the drainage map out in the docs, and it looks like it has the direction, but I am not sure that it captures all upstream cells correctly.

[sallymatson]

LGTM