Added mycorrhizal fungi to the soil model#834
Conversation
…tal per taxonomic group, rather than per functional group
… can handle external carbon supply
…rients from microbes
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
dalonsoa
left a comment
There was a problem hiding this comment.
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.
|
@dalonsoa ahh yes sorry about the size of the PR! I would say that the substantive changes in this PR are in the 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! |
dalonsoa
left a comment
There was a problem hiding this comment.
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) |
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.uptaketo 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
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks