Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1015 +/- ##
===========================================
- Coverage 94.60% 92.48% -2.12%
===========================================
Files 79 81 +2
Lines 6937 7096 +159
===========================================
Hits 6563 6563
- Misses 374 533 +159 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I've run through the sketch in v2 and tried to unpack it into a working model of a draft API in plant_resources_v3.py. If I understand correctly, we need to:
- Identify a set of variables in data that provide a resource mass and some metric of elemental composition (masses or ratios - we could probably use either).
- Identify a variable used to track consumed mass from that resource.
- The resource is always structured by cell_id but may also be structured vertically and by PFT. We probably need to keep PFTs as separate resources.
- We then need a mechanism by which an array can be parcelled out as individual cells using something that supports the Resource protocol.
- We then need to be able to pull consumed mass back across cells and write it back to data.
The file I've pushed hinges on two key mechanisms:
- The ArrayResources class can be used to produce cell specific resources using indexing: so
plant_resource_0 = array_resource_instance[0]gives a PlantResource for cell 0. - The total mass consumed in
plant_resource_0is a sneaky view back into an array across cells in the originating ArrayResources instance - so the total mass consumed is auto compiled.
The file contains some code at the end showing a rough workflow for the new classes. I think this should drop into your existing framework relatively easily?
@jacobcook1995 How does this compare to what is being used for soil array resources - can we unify to a common standard here?
@dalonsoa Could you have a quick look at the logic here - I think using array views to track the data generated by the child PlantResource back into the parent ArrayResources seems sensible but are there better ways to achieve this?
| self.mass_stoich: dict[str, float] = { | ||
| "carbon": 0.0, | ||
| "nitrogen": 0.0, | ||
| "phosphorus": 0.0, | ||
| } |
There was a problem hiding this comment.
Is it necessary to track this? The get_eaten() method tracks off mass_current and the cnp_proportions - it doesn't seem like these masses need to be tracked separately (unless it is used for targeting resources, but I would think that animals target resources based on their expectations of stochiometry, rather than rather god-like knowledge of the total pool).
There was a problem hiding this comment.
A dictionary of this kind is just the standard format of passing mass around in the animal model. The way I did CNP in the plant get_eaten is a non-standard implementation. I wasn't sure how you were handling CNP exactly so I built in the bits required for either route. Ultimately, as long as get_eaten returns a dict of that kind, it should work for the animal model.
There was a problem hiding this comment.
I wasn't sure how you were handling CNP exactly so I built in the bits required for either route.
Neither am I, yet 😄
However it comes in (mass or ratio) I think handling it internally as proportions and only tracking total mass is easier. It's not like the animals can alter the ratios? 🤞
|
This looks like a very promising way to break the flow down! I'll leave fiddling with foraging interface for after we hear from Jacob and Diego. |
| self.n_ratio_var = n_ratio_var | ||
| self.p_ratio_var = p_ratio_var |
There was a problem hiding this comment.
Can we name these CN & CP ratios just for continuity?
|
@TaranRallings Do you prefer to deal with masses of C, N, P or - as is the case in the code at the moment - with carbon mass and then CN and CP ratios? @sallymatson and I want to standardise how we write C+N+P to the |
|
So the above could definitely work for the litter pools, and seems like a much cleaner approach. I think it also could work for the soil pom pool. With some basic modification I think it could work for the soil bacteria pool (the issue there is that the stoichiometry comes is a constant rather than being recorded in the data object). Getting it to work for the soil fungi pool would be much more tricky, because in that case you have to combine three fungal pools into a single animal available pool (+ weighting the stoichiometric ratios appropriately) Fungal fruiting bodies would need a pretty big refactor to work in this framework (because they are primarily updated in the animal model, which could change but that would require thought). Carcasses and excrement would have a similar problem I think Given all that I'm not sure what the best approach is. I think it definitely makes sense to treat litter pools as plant resources, that follow the same framework. But I'm unsure whether it makes more sense to try and fit the other pools within the same overarching framework, or just to treat them as something meaningfully different that is handled differently? |
dalonsoa
left a comment
dalonsoa
left a comment
There was a problem hiding this comment.
Sorry for the slow reply - I hadn't spotted this.
The approach looks quite sensible and I do not see any obvious improvement. I have a couple of comments, though, because I'm not entirely sure where this would be used.
| for var in (mass_var, n_ratio_var, p_ratio_var, mass_consumed_var): | ||
| if var not in data: | ||
| raise ValueError( | ||
| f"Cannot initialise ArrayResource: {var} not found in data object" | ||
| ) |
There was a problem hiding this comment.
This should not be needed. This resource would be initialised by a model - presumably the AnimalModel - which will have among its required variables (possibly required by init ones?) the ones indicated here. So, by this point, all this validation should have been done already.
There was a problem hiding this comment.
True - there's a point at which putting belt and braces of error checking on all your code makes it slow down too much. We could lose this - any crash is then a result of the code of the AnimalModel being wrong - it isn't a user facing calibration.
| # Then within a single time loop | ||
| for cell_id in np.arange(12): | ||
| # Get the cell level resource for this cell from each array resource | ||
| cell_resources = [array_res[cell_id] for array_res in herbivory_resources] | ||
|
|
||
| # All consumers feed on all four resources | ||
| for herbivore in [consumer] * 4: | ||
| for resource in cell_resources: | ||
| resource.get_eaten(consumed_mass=rng.random(), consumer=herbivore) | ||
|
|
||
| # After the loop finishes print out the accumulated herbivory in the ArrayResource | ||
| # instances | ||
| for array_res in herbivory_resources: | ||
| print(array_res) | ||
| print(array_res.consumed_mass) | ||
|
|
||
| # The data object still doesn't know about herbivory | ||
| data["leaf_mass_consumed"] | ||
| data["subcanopy_mass_consumed"] | ||
|
|
||
| # So use the write_herbivory() to push the accumualate value out to the Data object. | ||
| # Note that the PFT specific resources are writing to different parts of the same data | ||
| # array. | ||
| for array_res in herbivory_resources: | ||
| array_res.write_herbivory() | ||
|
|
||
| data["leaf_mass_consumed"] | ||
| data["subcanopy_mass_consumed"] | ||
|
|
||
| # And then, before starting the next loop: | ||
|
|
||
| for array_res in herbivory_resources: | ||
| array_res.set_mass_and_elemental_ratios() |
There was a problem hiding this comment.
Where would all of these be happening? Within the AnimalModel update or somewhere else?
Also, that triply nested for loop looks super inefficient. although it is not obvious how to improve it as none of the functionality is vectorised...
There was a problem hiding this comment.
From this line down, it would be within AnimalModel._update (although probably as a call to an encapsulating method?):
# Get a tuple of all herbivory array resources
Everything above that is the PlantsModel pushing Plant data into the Data object, in PlantsModel.__init__ and PlantsModel._update
And I agree the triply nested loop looks bad but we'd need to vectorise the animal model to avoid it.
| mass_consumed_var="subcanopy_seedbank_biomass_consumed", | ||
| vertical_occupancy=VerticalOccupancy.GROUND, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
I'd probably do this - should be easier to maintain?
| ) | |
| # TODO: plant resource | |
| plant_resources = [ | |
| ( | |
| "subcanopy_vegetation_biomass", | |
| "subcanopy_vegetation_n_ratio", | |
| "subcanopy_vegetation_p_ratio", | |
| "subcanopy_vegetation_biomass_consumed", | |
| VerticalOccupancy.GROUND, | |
| ), | |
| ( | |
| "subcanopy_seedbank_biomass", | |
| "subcanopy_seedbank_n_ratio", | |
| "subcanopy_seedbank_p_ratio", | |
| "subcanopy_seedbank_biomass_consumed", | |
| VerticalOccupancy.GROUND, | |
| ), | |
| ] | |
| self._self._plant_array_resources = ( | |
| ArrayResources( | |
| data=self.data, | |
| mass_var=mvar, | |
| n_ratio_var=nvar, | |
| p_ratio_var=pvar, | |
| mass_consumed_var=mcvar, | |
| vertical_occupancy=vocc | |
| ) for mvar, nvar, pvar, mcvar, vocc in plant_resources | |
| ) | |
There was a problem hiding this comment.
Oh yeah, that's much better!
|
I'm closing this as it is superseded by #1318 |
6 participants
Description
This is a draft of a new plant resources interface for sitting inside the animal model. I would like it to do a few things:
I've tried fiddling with the data import stuff on my own but I think it will be much more efficient if you can help me out on that front. I have no problem doing all of the foraging interface but the plant data end is pretty arcane for me.
Can you provide feedback on:
_extract_mass_from_datamethod (or a better solution for the same problem)Thanks!
Fixes # (issue)
Type of change
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks