Group internal attributes in canopy into data classes

[davidorme]

Description

This PR revises the internal structure of the Canopy class, grouping the attributes under two new dataclasses: CohortCanopyData and CommunityCanopyData, which use __post_init__ methods to populate the attributes. This:

• gives a cleaner internal structure
• groups the attributes that share the same dimensions (cohort is (n_heights, n_cohorts), community is just (n_heights,))
• allows us to get these dataclasses to inherit methods with shared functionality such as export to pandas (upcoming Shared base classes for demography classes #338).
• simplifies the internals of Canopy._calculate_canopy by bundling cohort and community calculations. This is something that @j-emberton requested in PR Revise and document functionality of canopy and crown #328.
• adds a new test class to validate the calculations in these two dataclasses, which is a finer grained check than the previous implementation.

The calculation order here goes:

1. main cohort attributes on light extinction
2. whole community light extinction and fraction absorbed
3. divide the whole community absorption among cohorts and stems

The first iteration had: CohortCanopyData, CommunityCanopyData and then CohortCanopyData.allocate_fapar method, but that leaves some canopy attributes hanging until the allocation method is called. The code now generates the CommunityCanopyData within CohortCanopyData so it all happens in a single pass. Within Canopy the community data is moved up into a top level attribute for ease of access.

self.cohort_data = CohortCanopyData(...)
self.community_data = self.cohort_data.community_data

Fixes #337

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.95%. Comparing base (1f315ba) to head (34509eb).
Report is 305 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #341      +/-   ##
===========================================
- Coverage    95.29%   94.95%   -0.34%     
===========================================
  Files           28       34       +6     
  Lines         1720     2577     +857     
===========================================
+ Hits          1639     2447     +808     
- Misses          81      130      +49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[j-emberton]

seems reasonable to me

[davidorme]

@j-emberton I'm just looking at #314 and that alternative arrangement of calculating everything within CohortCanopyData seems cleaner now:

self.cohort_data = CohortCanopyData(...)
self.community_data = self.cohort_data.community_data

This is mostly because it won't leave the cohort_fapar and stem_fapar attributes hanging undefined. That just seems like an error waiting to happen (and also makes the to_pandas method ugly because not all of the variables that could possibly by exported are definitely there in an instance.

I could define those attrs with a default or get the pandas exporter to check that each exported attribute actually exists on an instance, but it seems easier just to go with that alternative.

Any reasons not to? I've pushed the updated implementation here and the specific changes are: a82c6ea...0ba5f11

[j-emberton]

@j-emberton I'm just looking at #314 and that alternative arrangement of calculating everything within CohortCanopyData seems cleaner now:

self.cohort_data = CohortCanopyData(...)
self.community_data = self.cohort_data.community_data

This is mostly because it won't leave the cohort_fapar and stem_fapar attributes hanging undefined. That just seems like an error waiting to happen (and also makes the to_pandas method ugly because not all of the variables that could possibly by exported are definitely there in an instance.

I could define those attrs with a default or get the pandas exporter to check that each exported attribute actually exists on an instance, but it seems easier just to go with that alternative.

Any reasons not to? I've pushed the updated implementation here and the specific changes are: a82c6ea...0ba5f11

Yeah I can't see any reason not to do this. Happy for you to keep going.