Skip to content

Structure for Extra PFT Traits.#943

Merged
sallymatson merged 7 commits intodevelopfrom
899-make-ideal-foliage-cnp-ratios-pft-dependent
Jul 24, 2025
Merged

Structure for Extra PFT Traits.#943
sallymatson merged 7 commits intodevelopfrom
899-make-ideal-foliage-cnp-ratios-pft-dependent

Conversation

@sallymatson
Copy link
Collaborator

@sallymatson sallymatson commented Jul 10, 2025

Description

I've now completed a draft of the ExtraTraitsPFT class. The initial goal of this class is to allow us to define stoichiometric ratios for plant tissues by PFT (before, they were all set to standard defaults in the plant constants.) To do this I have:

  • Added a class (ExtraTraitsPFT) which stores a dictionary (keyed by PFT name) of dictionaries (keyed by traits). This is used in parallel with the Flora model, meaning that it stores these values per PFT, but it does not store any information about cohorts, grid cells, etc so it is essentially a reference dictionary for the entirety of the simulation.
{'pft_name': {'trait_one': value, 'trait_two': value, ...}, 'pft_name_2': {'trait_one': value, 'trait_two': value, ...} ...}
  • Expanded the get_flora_from_config method to return both a Flora instance and an ExtraTraitsPFT instance, and made that correction throughout the model. This part is a bit hacky, as it takes a pre-defined list of "stochiometry traits" to separate out to set up the ExtraTraitsPFT and uses the rest for the Flora. It works for now but it's a bit hardcoded, so would love thoughts on if we should make this more flexible, or if it's ok given that we do want to obstruct some of these things from the user (the user should just give all the traits for a PFT in the schema, and let US decide which belong where, as it's a somewhat arbitrary dictinction between what is in pyrealm and what is not.)
  • Removed the stoichiometric ratios from PlantConsts and removed relevant references in the plants model and stoichiometry model.

Fixes # (issue)

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

@sallymatson sallymatson linked an issue Jul 10, 2025 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2025

Codecov Report

Attention: Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.22%. Comparing base (ed350be) to head (a99e889).

Files with missing lines Patch % Lines
...irtual_ecosystem/models/plants/functional_types.py 93.75% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #943   +/-   ##
========================================
  Coverage    94.21%   94.22%           
========================================
  Files           79       79           
  Lines         6518     6563   +45     
========================================
+ Hits          6141     6184   +43     
- Misses         377      379    +2     

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

"foliage_c_n_ratio",
"foliage_c_p_ratio",
]

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about lignin content for the different tissues, as well as carbon mass per propagule, where are these located? Are these still seen as constants?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are currently still set in the constants, in the plant consts model.

I guess at some point we would break that out as well, and ExtraTraitsPFT can be used for that.

self.data["deadwood_c_n_ratio"] = xr.full_like(
self.data["elevation"], self.model_constants.deadwood_c_n_ratio
)
self.data["deadwood_c_n_ratio"] = xr.full_like(self.data["elevation"], 56.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you help me understand what this does:
self.data["deadwood_c_n_ratio"] = xr.full_like(self.data["elevation"], 56.5)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This uses default values for the tissue variables, to get passed to @jacobcook1995 . This needs to be dynamic - however I am hoping to do that in another PR. Also need to discuss exactly how to calculate those values. Will add a comment noting this todo!

Comment on lines +79 to +81
reclaim_ratio=np.array(
[
extra_pft_traits.traits[name]["leaf_turnover_c_n_ratio"]
Copy link
Collaborator

@arne-exe arne-exe Jul 17, 2025

Choose a reason for hiding this comment

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

What does reclaim_ratio stand for (i.e., how does it behave in the model)?
-Is it to calculate the amount of nutrients that are reclaimed (and thus kept within the tree) before leaves are lost due to turnover? If so, would this mean we need both a (senesced) leaf_c_n_ratio and leaf_turnover_c_n_ratio to calculate their difference?
-I vaguely remember reading about how the nutrient cost to replace turnover must be taken into account, so perhaps that's what this reclaim_ratio does? Could you please remind me again how this works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly (nutrients reclaimed). There are two separate ratios - the foliage ratio (healthy) and the turnover (senseced). It's a bit confusing, so this is something that I will need to eventually clarify in the docs.

I definitely owe a big doc refresh for all of stochiometry - will add an issue for this.

Copy link
Collaborator

@arne-exe arne-exe Jul 29, 2025

Choose a reason for hiding this comment

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

Hi @sallymatson, thanks for the clarification. I agree it's a bit difficult to understand what is being referred to with turnover (senesced) leaves. Are they already considered turnover/litter? Has nutrient resorption already taken place (I assume we are talking about adult/mature leaves here, or are you referring to something more specific with senesced?). Should nutrient resorption be the difference between the nutrient content of senesced (i.e., mature/adult) leaves and leaf turnover/litter nutrient content (in my opinion this is how nutrient resorption should be calculated). I think most of the data on leaf nutrient content for Borneo comes from chemical analysis on leaf litter, so working with an average ratio of fresh leaves-litter nutrient content sounds like the likely way to go.

On second thought, it seems likely that senesced already refers post resorption, as else I don't see why we'd also have the other "healthy" leaves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is that the plants model calculates how much turnover there is in each timestep. This gives us the amount of leaves that will senesce, and therefore pass into the litter model. These leaves are never part of the tree so leaves are never "considered" senesced, however since the tree reclaims nutrients, we need to know the difference in the stochiometric ratio for the two different forms.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These leaves are never part of the tree

They kinda are 😄 .

On second thought, it seems likely that senesced already refers post resorption, as else I don't see why we'd also have the other "healthy" leaves.

That's correct. The leaf mass of the tree has a target stoichiometric ratio for leaf tissue in general. We don't currently have any kind of leaf age or leaf-economics model. The leaves try and track that ratio as closely as possible but may not be able to realise the ideal ratio if nutrients are limiting.

When we calculate the leaf mass turnover in a time step, there is immediately a resorption of nutrients using the reclaim ratio. The reclaimed nutrients are reallocated within the tree and the remaining nutrients are lost to litter.

Copy link
Collaborator

@arne-exe arne-exe Aug 8, 2025

Choose a reason for hiding this comment

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

Thanks for the clarification @sallymatson, this is very helpful.

These leaves are never part of the tree

They kinda are 😄 .

In theory they should contribute to foliage mass before they get turned into leaf litter, but I can see how working within a timestep leads to this approach. This does seem to make an important assumption about the link between litterfall timing and the timestep length, but I guess since turnover is scaled according to time (i.e., per year) this mostly solves that issue?

That's correct. The leaf mass of the tree has a target stoichiometric ratio for leaf tissue in general. We don't currently have any kind of leaf age or leaf-economics model. The leaves try and track that ratio as closely as possible but may not be able to realise the ideal ratio if nutrients are limiting.

When we calculate the leaf mass turnover in a time step, there is immediately a resorption of nutrients using the reclaim ratio. The reclaimed nutrients are reallocated within the tree and the remaining nutrients are lost to litter.

Also, thanks for clarifying this @davidorme.

Some additional thoughts: It's interesting to think about how nutrient limitation may affect other processes in the model. For example, it seems likely that, apart from lower tissue nutrient content, the allocation in the T model may change, and GPP would also change depending on leaf nutrient content. The same goes for when there is an excess in nutrients, would a tree choose to create more nutrient rich tissues (as currently implemented), or would it create more tissues (e.g., invest in greater leaf area). These processes are likely to be quite complex and influenced by many things (like environment, competition, etc.).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes... that's definitely something we've discussed. We've come to the conclusion that resource competition is currently WAY too complex, but it's critical for the next step of the model (i.e. after our first runs/prototypes.) If you have knowledge in that area it'll be super helpful when we begin to implement it.

name,a_hd,ca_ratio,h_max,lai,par_ext,resp_f,resp_r,resp_s,rho_s,sla,tau_f,tau_r,yld,zeta,f_g,m,n,tau_rt,resp_rt,gpp_topslice,p_foliage_for_reproductive_tissue
test1,116.0,390.43,25.33,1.8,0.5,0.1,0.913,0.044,200.0,14.0,4.0,1.04,0.6,0.17,0.02,2,5,1,0.05,0.1,0.05
test2,116.0,390.43,15.33,1.8,0.5,0.1,0.913,0.044,200.0,14.0,4.0,1.04,0.6,0.17,0.02,2,5,1,0.05,0.1,0.05 No newline at end of file
name,a_hd,ca_ratio,h_max,lai,par_ext,resp_f,resp_r,resp_s,rho_s,sla,tau_f,tau_r,yld,zeta,f_g,m,n,tau_rt,resp_rt,gpp_topslice,p_foliage_for_reproductive_tissue,deadwood_c_n_ratio,deadwood_c_p_ratio,leaf_turnover_c_n_ratio,leaf_turnover_c_p_ratio,plant_reproductive_tissue_turnover_c_n_ratio,plant_reproductive_tissue_turnover_c_p_ratio,root_turnover_c_p_ratio,root_turnover_c_n_ratio,foliage_c_n_ratio,foliage_c_p_ratio
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should lignin content of tissues and carbon mass per propagule also be here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eventually yes - for now it is out of scope of this PR / where we are at rn.

@arne-exe
Copy link
Collaborator

Hi @sallymatson, it looks good to me. I left a few questions about tissue lignin content and carbon mass per propagule, and some questions about what some parts of the code do but other than that no remarks from me.

Copy link
Collaborator

@davidorme davidorme 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 but I think there is an issue with the class method on the ABC.

ABC objects are not intended to be directly used and all functionality should be through subclasses. The from_pft_default_ratios @classmethod should be an @abstract_method of Tissue and then the individual tissue subclasses should implement their own methods. This isn't exactly like you are creating an instance of Tissue - which is definitely not intended with ABCs - but the class method relies on the definition of specific downstream subclasses. If we wanted to add a new Tissue, we'd have to edit the ABC to support it. I can see why you've done it - because then you have the code to index the ratios from PFT names in a single location - but it isn't a clean structure.

I think there's an argument for making ExtraPFTTraits more like Flora, so it can use the CohortMethods and PandasExporter mixins from pyrealm. Then we could keep the traits in line with the community PFTs using the same mechanism and wouldn't have to do that alignment. But that might be a tweak for a separate PR.

Comment on lines +49 to +56
@classmethod
def from_pft_default_ratios(
cls,
subclass,
community: Community,
extra_pft_traits: ExtraTraitsPFT,
ratio_name: str,
) -> "Tissue":
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should become an abstract method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@davidorme updated - can you take a look and see if it's what you intended?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup - that's what I intended 😄.

I vaguely wonder if there's an argument for nesting dicts within ExtraTraits so we could do:

extra_pft_traits.traits[name][element_name]["foliage_c_ratio"]

But that seems like something to circle back to and might clash with making ExtraTraitsPFT use CohortMethods etc later

@davidorme
Copy link
Collaborator

  • Expanded the get_flora_from_config method to return both a Flora instance and an ExtraTraitsPFT instance, and made that correction throughout the model. This part is a bit hacky, as it takes a pre-defined list of "stochiometry traits" to separate out to set up the ExtraTraitsPFT and uses the rest for the Flora. It works for now but it's a bit hardcoded, so would love thoughts on if we should make this more flexible, or if it's ok given that we do want to obstruct some of these things from the user (the user should just give all the traits for a PFT in the schema, and let US decide which belong where, as it's a somewhat arbitrary dictinction between what is in pyrealm and what is not.)

I think it is absolutely fine to abstract that detail from the users. If we wanted to get a bit cleaner, Flora has an array_attrs class attribute that we could use to define that subset of the traits ( but that isn't really anything but a slightly fancier list of traits 😄 ).

@sallymatson sallymatson requested a review from davidorme July 23, 2025 11:08
Copy link
Collaborator

@davidorme davidorme left a comment

Choose a reason for hiding this comment

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

LGTM

@sallymatson sallymatson marked this pull request as ready for review July 24, 2025 09:27
@sallymatson sallymatson merged commit 7dc92cf into develop Jul 24, 2025
16 checks passed
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.

Make ideal foliage CNP ratios PFT dependent

4 participants