Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

703 adding growth to the plantsmodel #730

Draft
wants to merge 4 commits into
base: 659-use-pyrealm-flora
Choose a base branch
from

Conversation

sallymatson
Copy link
Collaborator

@sallymatson sallymatson commented Feb 6, 2025

Description

Adding functionality to allocate_gpp, which takes the gpp and then grows the plants 😄

Fixes #703

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

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.87%. Comparing base (d2de394) to head (c8cfd42).
Report is 27 commits behind head on 659-use-pyrealm-flora.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           659-use-pyrealm-flora     #730   +/-   ##
======================================================
  Coverage                  94.86%   94.87%           
======================================================
  Files                         73       73           
  Lines                       4926     4935    +9     
======================================================
+ Hits                        4673     4682    +9     
  Misses                       253      253           

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

@sallymatson sallymatson requested a review from davidorme February 7, 2025 13:07
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.

A couple of comments, but looks good so far.

Comment on lines +174 to +175
# .. TODO: not sure what to test here? Main thing it does is change the
# dbh value, so maybe check that it has grown?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha! The .. TODO:: annotation is specific to function docstrings, so they appear nicely styled in the HTML docs. Here, plain old TODO will cover it 😄

On the question though - yes, just checking they are larger than their state before the call seems reasonable.


# Check that leaf_turnover and root_turnover are correctly calculated
for cell_id in fxt_plants_model.communities.keys():
# .. TODO: same as above, this is a bit logic-less
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 is hard to avoid though, we could set up something testing for specific values but that is going to be a pain to do.

We could generate some specific numeric expectations but that does feel a bit circular.

Comment on lines +576 to +578
cohort_allometry = StemAllometry(
stem_traits=community.stem_traits, at_dbh=cohorts.dbh_values
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually already populated in community.stem_allometry but at the moment there's no mechanism by which that gets updated when the DBH increases.

Comment on lines +586 to +587
# Grow the plants by increasing cohort dbh
cohorts.dbh_values += cohort_allocation.delta_dbh[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should need that [0]? It should generate an array that maps 1-to-1 onto the cohorts?

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.

3 participants