Skip to content

Added mycorrhizal fungi to the soil model#834

Merged
jacobcook1995 merged 16 commits intodevelopfrom
805-mycorrhizal-fungi
May 8, 2025
Merged

Added mycorrhizal fungi to the soil model#834
jacobcook1995 merged 16 commits intodevelopfrom
805-mycorrhizal-fungi

Conversation

@jacobcook1995
Copy link
Copy Markdown
Collaborator

Description

This PR adds mycorrhizal fungi to the soil model (as two additional microbial groups arbuscular mycorrhizal and ectomycorrhizal). Implementing this required some reorganisation of the code. The main addition is a new submodule soil.uptake to perform the calculations related to microbial uptake (this was previously a single function which became unmanageable).

Let me know if there's anything else I can do to make the code cleaner/more maintainable.

Also @davidorme and @sallymatson this PR makes changes to the plant model (in line with our previous discussions), but if anything looks off to you or conflicting with stuff you are adding let me know!

Fixes #805

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

@jacobcook1995 jacobcook1995 linked an issue Apr 24, 2025 that may be closed by this pull request
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.75%. Comparing base (0e7fdbd) to head (174c576).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #834      +/-   ##
===========================================
+ Coverage    94.60%   94.75%   +0.15%     
===========================================
  Files           74       75       +1     
  Lines         5394     5550     +156     
===========================================
+ Hits          5103     5259     +156     
  Misses         291      291              

☔ 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.

Copy link
Copy Markdown
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

This is a pretty large PR, so is there a specific part you want your feedback on?

Aside from the review itself, and being totally out of my domain here, how generic is what you are implementing? I mean, is it like having specific code for elephants, specific code for lions, etc., or more generic like behavior for mammals, etc.? How many more of these will need to be implemented?

I'm asking because if it is very specific and every time a new fungi or bacteria is added requires new code, this is going to make the whole thing really complicated really fast, and very much not modular.

@jacobcook1995
Copy link
Copy Markdown
Collaborator Author

@dalonsoa ahh yes sorry about the size of the PR! I would say that the substantive changes in this PR are in the calculate_microbial_changes function in pools.py and the new updake.py submodule. I think these would be the most useful parts to get your feedback on.

The other changes to the code were a small extension to the existing microbial functional group setup (which has already been discussed quite a bit in previous PRs) and changes to the plant model code to get it to work with the soil model changes (@davidorme and @sallymatson are probably much better placed to give feedback on this).

The code genericness question is quite tricky. Given the lack of data on soil microbes we are trying to limit the number of functional groups we include. The criteria for adding another functional group is basically "does this group interact with the soil environment by a different mechanism rather than a different parameterisation of the same mechanism". This means that every addition of a new functional group has required new functionality. (with the caveat that arbuscular and ecto-mycorrhizal fungi use the same mechanisms, but this is because there is a lot more data on this specific split).

I don't feel like it makes sense to give the user the ability to define arbitrary functional groups, and I am hoping we can keep the total number of functional groups at 4 (i.e. the amount this PR brings it up to). Though we might have add nitrogen fixing bacteria as an explicit functional group in future (if the current simplified form they exist in doesn't fly). But regardless we should be very near to the maximum complexity here. Any suggestions on trying to make the code more generic would be really helpful though!

@jacobcook1995 jacobcook1995 requested a review from dalonsoa May 6, 2025 12:14
Copy link
Copy Markdown
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

Overall, this looks OK and I do not see any obvious area of improvement. My only concern is the same one that I have in a previous PR, how this is to scale if more groups need to be added. How many more groups are there?

@jacobcook1995
Copy link
Copy Markdown
Collaborator Author

Overall, this looks OK and I do not see any obvious area of improvement. My only concern is the same one that I have in a previous PR, how this is to scale if more groups need to be added. How many more groups are there?

The current plan is to keep the number of functional groups at the current 4 groups. I completely agree that if this changes in the future a more general approach will be needed. I think it makes sense to treat that as a problem that may never happen for now though (but obviously to bear scalability in mind when code refactors happen)

Copy link
Copy Markdown
Collaborator

@sallymatson sallymatson left a comment

Choose a reason for hiding this comment

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

This looks good, as discussed with the plant and soil interactions. Thanks Jacob!

@jacobcook1995 jacobcook1995 merged commit 2ec7f4a into develop May 8, 2025
16 checks passed
@jacobcook1995 jacobcook1995 deleted the 805-mycorrhizal-fungi branch May 8, 2025 09:03
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.

Mycorrhizal fungi

4 participants