ImperialCollegeLondon / pyrealm

Development of the pyrealm package, providing an integrated toolbox for modelling plant productivity, growth and demography using Python.
https://pyrealm.readthedocs.io/
MIT License
16 stars 6 forks source link

WIP - 230 create a draft canopy class #231

Open AmyOctoCat opened 2 months ago

AmyOctoCat commented 2 months ago

Description

An initial implementation for a canopy class that calculates the canopy layer heights for a given community. See community_sketch.py for an example of the code in use.

Still to do:

A rough outline of how the canopy layers are calculated:

  1. Data is imported (Cohort and Community classes can be instantiated in code or imported from json files using the deserialisation package), in the form of a json array of ImportedCommunity objects, and a json array of PlantFunctionalType objects.
  2. The deserialiser then converts these into Community objects. The Community class is effectively a container for a list of cohorts, a cell_id and cell_area.
  3. When the Cohorts are initialised as part of the deserialisation, the T Model Geometry and Canopy Factors from Jaideep's extension to the T Model are calculated.
  4. A Canopy class can then be created from a Community object and a canopy gap fraction.
  5. This has a calculate_layer_heights method, which is called during initialisation of the Canopy class.

Notes:

Fixes # (issue)

Type of change

Key checklist

Further checks

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 53.84615% with 48 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (develop@c53eb92). Click here to learn what that means.

Files Patch % Lines
pyrealm/community_sketch.py 53.84% 48 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #231 +/- ## ========================================== Coverage ? 92.85% ========================================== Files ? 29 Lines ? 1805 Branches ? 0 ========================================== Hits ? 1676 Misses ? 129 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AmyOctoCat commented 1 month ago

I think this is good but there are some structural things it would be good to look at. A couple of overarching things:

1. As noted below, I think I've given you a bad steer on the Community structure. I'm not convinced that actual `Cohort` objects are the way forward - the structure is very clear but it's likely to be less efficient. It is possible though that adding and deleting cohorts is much more efficient with lists of `Cohort` instances than subsetting and appending arrays?

2. I think I'd rather have factory methods of classes than the intermediate import classes - it just seems cleaner.

Hi David,

Thank you very much for the feedback, these were things I was going round in circles a bit myself, so it's great to have the input.

Regarding the first point I think I've circled round on the arrays vs objects quite a few times over and still don't have a strong feeling. I think going for the OO approach might make it easier to read and easier to reason about when it comes to adding and removing cohorts, but I can see the efficiency argument for using arrays.

Regarding the second point I also went in and out on this. JSON dataclasses did not play very nicely with passing the flora object into the nested cohorts within the community which was why I ended up with the separate import classes (I thought this was also a bit clearer from the point of view of easily seeing which fields were needed in the input data). Once there were separate import classes, the logic looked quite complicated and I thought it deserved it's own class (was going for the single responsibility principle from SOLID), but will have a review to see if there's a better way. Maybe adding extra functionality to the generated from_json method that json_dataclass creates in the cohort class is possible that would allow us to pass the flora in there.

Sorry that's all very inconclusive! Hopefully some more ideas might percolate over the space of the weekend.

vgro commented 1 month ago

This looks very good to me, Amy. I cannot say much about the performance, just a few comments on readability. I think it was said before, maybe in a different context, that it is easier to read if variable names have meaningful names, for example in solve_canopy_closure_height, see inline comment. I'm a bit unhappy with the z_m name for height where canopy radius is maximum, could you rename it to either z_max or something even more descriptive like canopy_radius_max_height. Also, z_m It is the standard name for roughness length of momentum which might come in at some point.