Skip to content
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

Conversation

jacobcook1995
Copy link
Collaborator

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

  • 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
Copy link

codecov-commenter commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.96%. Comparing base (3fcb8a7) to head (7a17d21).

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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@vgro vgro left a 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)

@jacobcook1995 jacobcook1995 requested a review from vgro August 1, 2024 13:49
"""

temperatures = {
"surface": air_temperatures[layer_structure.index_surface_scalar].to_numpy(),
Copy link
Collaborator

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?

Copy link
Collaborator Author

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)

@jacobcook1995 jacobcook1995 requested a review from vgro August 2, 2024 08:12
Copy link
Collaborator

@vgro vgro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jacobcook1995 jacobcook1995 merged commit 72ef3de into develop Aug 2, 2024
12 checks passed
@jacobcook1995 jacobcook1995 deleted the 532-litter-model-should-average-abiotic-quantities-over-relevant-soil-layers branch August 2, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Litter model should average abiotic quantities over relevant soil layers
3 participants