Feature/horizontal flow

[vgro]

This PR introduces horizontal flow of water in the hydrology model (accumulated surface runoff only).

At the moment, this means that upstream neighbours are identified in setup(), and at each time step, the surface runoff from the upstream neighbours is added to the runoff of each grid cell. This should be working for runoff provided by SPLASH. Below ground flow is not yet implemented.

I added recipes to make dummy elevation and surface_runoff data sets. I purposely didn't add the surface runoff to climate data because this might come from SPLASH soon.

I also changed the description of soil moisture to 'Volumetric relative water content (unitless)' to be consistent with the soil model.

There are still a lot of TODOs regarding spinup, boundaries, and below ground flow which I will address later.

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

[vgro]

I accidentally made a bit of a mess with poetry, please help to get around merging this

[codecov-commenter]

Codecov Report

Merging #293 (acb29d1) into develop (f44d9b3) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #293      +/-   ##
===========================================
+ Coverage    97.49%   97.54%   +0.04%     
===========================================
  Files           46       46              
  Lines         2114     2155      +41     
===========================================
+ Hits          2061     2102      +41     
  Misses          53       53              

Files Changed	Coverage Δ
virtual_rainforest/models/hydrology/constants.py	100.00% <ø> (ø)
...ual_rainforest/models/hydrology/hydrology_model.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[alexdewar]

I've given a few queries/suggestions, but it otherwise looks ok to me (notwithstanding that I don't understand the science!).

[alexdewar]

Is there a reason why these todos can't be done in this PR too? In particular, it seems like it should be easy to check for negative numbers. If you're not sure exactly what you want to do when you detect one, you could just raise an error for now and figure out how to handle it more gracefully in future.

Again, I think this block should be separated out from the rest of this function, either into a function with the rest of the code above or into its own function.

[davidorme]

This is definitely jumping the gun, but we could have a set of [0,1] coefficients here to allow partial draining into different cells 😄

[vgro]

@davidorme partial drainage is something for the below-ground flow I guess, which is not yet implemented. We can discuss that at a later stage ;-)

[alexdewar]

I forgot to comment on the poetry.lock problem. As far as I can tell, you should be able to just git revert all your changes to it (in reverse order).

If that doesn't work, you can just copy the version on develop to get it back to how it was, either manually or with:

git show develop:./poetry.lock > poetry.lock

[jacobcook1995]

LGTM! I had a couple of queries about the science/approach, but on the code quality front I didn't notice anything that Alex hadn't already brought up

[davidorme]

LGTM too. It would be good to pop out those functions - for the first I think we want something like:

def calculate_drainage_map(grid: Grid, dem: DataArray) dict[int, tuple[int]]:

That should return a mapping that allows us to lookup which cells drain into each cell. To future proof it, the tuple members could be tuples of cell id and runoff fraction, but that seems excessive now!

[davidorme]

This is definitely jumping the gun, but we could have a set of [0,1] coefficients here to allow partial draining into different cells 😄

[vgro]

LGTM too. It would be good to pop out those functions - for the first I think we want something like:

def calculate_drainage_map(grid: Grid, dem: DataArray) dict[int, tuple[int]]:

That should return a mapping that allows us to lookup which cells drain into each cell. To future proof it, the tuple members could be tuples of cell id and runoff fraction, but that seems excessive now!

I saw that a bit too late for the last commit, but I think this is a good idea. I'll introduce something like this in the next iteration 👍

[vgro]

LGTM too. It would be good to pop out those functions - for the first I think we want something like:

def calculate_drainage_map(grid: Grid, dem: DataArray) dict[int, tuple[int]]:

That should return a mapping that allows us to lookup which cells drain into each cell. To future proof it, the tuple members could be tuples of cell id and runoff fraction, but that seems excessive now!

I saw that a bit too late for the last commit, but I think this is a good idea. I'll introduce something like this in the next iteration 👍

I have now implemented something like this but kept it in the hydrology model for now (in __init__). It is a bit messy with a mix of lists and dicts...it works but could be prettier.

[alexdewar]

I've made some minor suggestions, but it's otherwise looking good.

The only other minor comment I've got is that I think some of the types you're using could be made more specific. Various args are specified as dicts or lists, without any extra info about what type of dict etc. they are. If you do know this, then it's better to specify that it's e.g. a dict[int, str]. It means you get better type-checking with mypy and it also makes it easier to read.

I considered all suggested changes and dismiss to merge because this PR is blocking other PRs