Revise and document functionality of canopy and crown

[davidorme]

Description

This issue:

• Revises the crown and canopy modules. The main changes are

  • Move a canopy function from crown into canopy: solve_canopy_area_filling_height
  • Add a plotting helper function to crown: get_crown_xy. I'm not 100% convinced about the API of this yet, but think it's useful.
  • Revised the internals of the canopy model to try and generalise how it works and fixed some bugs.

• Updates the user facing documentation for the crown and canopy modules.

• Updates the existing tests to the new structures. Doesn't add any tests as yet.

Some things to note:

• I've made the open PR Calculate canopy light extinction and interception #318 outdated. That PR is currently hanging with errors that I think are fixed in here and this PR includes all the code from Calculate canopy light extinction and interception #318. That turned out to be necessary because I needed to fix the code to I write the docs.

• I've updated the way in which the metadata in the markdown notebooks is handled: jupyter itself can add metadata to notebooks and code cells. Because we are using Myst Markdown through jupytext, that has its own filtering mechanism that controls what actually gets preserved in the file. That's now caused all of our markdown files to get minor changes (hence the 47 files changed 😱 ).

  There are really only a few files that matter: the crown and canopy docs (docs/source/users/demography/..., the crown and canopy modules (pyrealm/demography/...) and the minor rearrangements in the tests.

---

Things to do (maybe not in this PR)

• There are a couple of areas in the code that I haven't resolved. These are described in TODO boxes here in the built_docs for this PR.
• There are some missing tests.
• The docs need updating to explain the things I haven't resolved, as and when they get resolved.

Will fix both #316 and #315

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

Attention: Patch coverage is 73.58491% with 28 lines in your changes missing coverage. Please review.

Project coverage is 94.90%. Comparing base (1f315ba) to head (55b6f90).
Report is 260 commits behind head on develop.

Files with missing lines	Patch %	Lines
pyrealm/demography/crown.py	30.55%	25 Missing ⚠️
pyrealm/demography/canopy.py	95.71%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #328      +/-   ##
===========================================
- Coverage    95.29%   94.90%   -0.39%     
===========================================
  Files           28       34       +6     
  Lines         1720     2554     +834     
===========================================
+ Hits          1639     2424     +785     
- Misses          81      130      +49     

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

[j-emberton]

I would prefer to see the individual operations in _calculate_canopy() broken out into separate methods for testability

[davidorme]

I can see why.

The reason I haven't is because there are two trivial calculations and then a chain of calculations that build on each other. It could be split into calculate_leaf_area_index and calculate_light_transmission but both of those would be much cleaner as methods of Canopy rather than standalone functions as the signatures are a bit unwieldy if we don't pass in the content of self.

If we do that then we have:

class Canopy:
    ...
    def _calculate_canopy(self, ...):
        ...
       self._calculate_leaf_area_index()
       self._calculate_light_transmission()

    def _calculate_leaf_area_index(self, ...):
        ...

    def _calculate_light_transmission(self, ...):
        ...

That's still all chained together - it's not immediately clear to me that there is a huge advantage. Did you mean to break them out into standalone functions?

[davidorme]

@j-emberton I think I'll leave it as is because I'm going to be revisiting this section in #337.

[j-emberton]

Ok. Just reviewing all this now.

[j-emberton]

Is this still draft PR? Aside from a little (suggested) restructuring in canopy it looks good to go

[davidorme]

That's tricky - this isn't complete or final.

• There's a section on the light allocation that is wrong because it ought to vary with crown gap fraction and doesn't.
• I'm not 100% sold on the workflow yet.
• There is Validate sign of inputs to T Model functions. #295 and Review validation and return shapes from demography objects #317 kicking around
• There isn't enough testing.

But it feels like it's just going to keep sprawling, so I'm tempted to address the issues and bank this and add bit size issues to fix the above. Thoughts?

[j-emberton]

This looks ok to me.

I 've added another comment regarding helping people understand what the list of valid str attributes that can be passed but its not a deal breaker.