-
Notifications
You must be signed in to change notification settings - Fork 1
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
532 litter model should average abiotic quantities over relevant soil layers #534
532 litter model should average abiotic quantities over relevant soil layers #534
Conversation
…ate environmental factors
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #534 +/- ##
===========================================
+ Coverage 94.93% 94.96% +0.02%
===========================================
Files 73 73
Lines 4048 4071 +23
===========================================
+ Hits 3843 3866 +23
Misses 205 205 ☔ 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.
Looks overall good to me, I like how the use of the LayerStructure components makes it much clearer. Also, I think the structure is becoming more and more clear as functions are grouped.
My main point to review is about how you calculate the temperature below ground, I think if you want a value along a gradient it would make sense to include the surface temperature (if the microbial activity depth is within the topsoil layer)
""" | ||
|
||
temperatures = { | ||
"surface": air_temperatures[layer_structure.index_surface_scalar].to_numpy(), |
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.
Just to check if I understand correctly, the "surface" value is for the above ground litter, not for the interpolation step?
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.
Yep this one is for the above ground litter, in this case I just use the surface temperature directly and don't interpolate (because that seems tricky without tracking litter depth etc)
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
Description
This pull request makes use of the new and improved
LayerStructure
object to average temperature and matric potential over the microbially active depth, rather than just taking the value from the topsoil layer. This has been done for just the litter model as that's the one I'm currently working on (but soil should follow a similar process when I get round to it see #533).This PR is generally pretty straightforward, the only slight weirdness being the testing. Basically the layer structure fixture has a really deep first soil layer, so it doesn't actually matter whether you average or just take the top layer as you get the exact same result. So I had to overwrite the fixture in some of the tests so that they actually test a case with averaging.
Fixes #532
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks