Conversation
…, a system for parsing input strings, and a broad_diet trait for linking to trophiclly specified constants.
…bring it inline with other animal facing resource pools.
…iring to connect waste, carcasses, and litter into the foraging loop.
…odified foraging methods to use them.
…ave associated methods.
TaranRallings
requested review from
Baizurah,
alexdewar,
dalonsoa,
davidorme,
nickwctan and
sallymatson
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #875 +/- ##
===========================================
- Coverage 94.80% 93.97% -0.83%
===========================================
Files 75 75
Lines 5599 5813 +214
===========================================
+ Hits 5308 5463 +155
- Misses 291 350 +59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacobcook1995
left a comment
jacobcook1995
left a comment
There was a problem hiding this comment.
The changes to the animal -> soil/litter links work for me. Just had one comment about litter wet mass
|
This is a pretty long PR, and I'm at a conference the whole of this week, so please bear with me and I will review it as soon as possible next week. |
dalonsoa
left a comment
dalonsoa
left a comment
There was a problem hiding this comment.
The code itself looks really good to me, and I only have a couple of questions and a suggestion to reduce the redundancy of the code.
|
|
||
| class DietType(Enum): | ||
| """Enumeration for diet types.""" | ||
| class DietType(Flag): |
| self.decomposed_cnp.update( | ||
| carbon=missed_cnp["carbon"], | ||
| nitrogen=missed_cnp["nitrogen"], | ||
| phosphorus=missed_cnp["phosphorus"], | ||
| ) |
There was a problem hiding this comment.
Just to confirm this is correct, it replaces the existing decomposed_snp, not added to it, right?
There was a problem hiding this comment.
The actual CNP values are stored in a CNP object which has that update method. The update method does add the value to the totals rather than replacing them.
There was a problem hiding this comment.
The intention is that decomposed_cnp is a pool of trophically inaccessible mass to which a portion of food that is attempted to eat falls. The pool is updated rather than replaced because this is a flow into the pool and jacob's soil model handles the flow out.
| self.decomposed_cnp.update( | ||
| carbon=missed_wet * frac_C, | ||
| nitrogen=missed_wet * frac_N, | ||
| phosphorus=missed_wet * frac_P, |
There was a problem hiding this comment.
This works the same way as above.
| """ | ||
| self.decomposed_cnp = CNP(carbon=0.0, nitrogen=0.0, phosphorus=0.0) | ||
|
|
||
| def get_eaten( |
There was a problem hiding this comment.
This function looks almost identical - haven't check exactly - to the other get_eaten above, so I would suggest to remove duplication. One way of doing it would be using mixins. It would look something like:
class GetEatenMixin:
def get_eaten(...):
# The implementation of the method, which looks identical.
@dataclass
class CarcassPool(GetEatenMixin):
...
@dataclass
class ExcrementPool(GetEatenMixin):
...I'm not entirely sure how mixins play with dataclasses, and you might need to get creative with mypy and use a Protocol for the mixin as described in mypy docs, but it should increase the re-usability of the code.
There was a problem hiding this comment.
If I understand your suggestion here, I think mixins and dataclasses work fine.
I haven't called them "mixins" but I believe that the PandasExporter and CohortMethods classes in pyrealm here are mixins:
https://github.com/ImperialCollegeLondon/pyrealm/blob/develop/pyrealm/demography/core.py
And I use them (multiply!) to share functionality among classes e.g. here
There was a problem hiding this comment.
Yep, they look the same thing.
There was a problem hiding this comment.
Although it seems like I don't need to declare them as an ABC? Will have to look!
There was a problem hiding this comment.
Ah, good idea. I hadn't seen those mixins before. I have replaced the redundant method with a mixin and the required protocol.
…ags instead of broad_diets.
…n and a supporting protocol.
6 participants
Description
This is the trophic expansion for the Animal Model. It's main job is to expand the types of diets that animals can have and provide the machinery for linking those diets to the right resources.
Core Changes
DietTypesinAnimalTraitsget_eatenmethods for carcass and waste poolsdelta_mass_andcalculate_consumed_mass_foraging methods for carcasses, detritus, and excrementprey_group_selectionscaling functionSupplemental
LitterPoolto align it with other resources - one object per grid cellcreate_new_cohortmethod toAnimalModelas I was repeating the same functionality in three placesFixes # (issue)
Type of change
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks