-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/vertical flow average #351
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #351 +/- ##
===========================================
- Coverage 95.91% 95.91% -0.01%
===========================================
Files 52 52
Lines 2596 2595 -1
===========================================
- Hits 2490 2489 -1
Misses 106 106 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just had a minor query. Also make sure to check that you are happy with my additions before you merge
@@ -402,7 +402,7 @@ def test_setup( | |||
coords={"cell_id": [0, 1, 2]}, | |||
) | |||
exp_vertical_flow = DataArray( | |||
[62.72513, 62.87226, 62.71498], | |||
[1.04541883, 1.04787099, 1.04524967], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor query, but why has this declined by a factor of 60 rather than a factor of 30?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. This comes because the sum was calculated over both soil layers, so it's twice the amount of water (which was incorrect). I now also corrected a small mistake in the averaging as I didn't stack the daily values first. So the vertical flow in the data object represents only the flow through the top soil layer. Is that sensible or should I keep it two-dimensional and you select the topsoil index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that seems sensible, I guess it makes sense to keep outputs simple unless someone needs a particular level of complexity
…perialCollegeLondon/virtual_rainforest into feature/vertical_flow_average
added comment to doc string about mean vertical flow in top soil layer
This PR changes the unit of the vertical flow in the data object from accumulated mm per time step (month) to mean mm per day. This is what the soil model needs as an input and it saved some conversions.
Fixes #347
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks