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

Initial plants model draft #296

Merged
merged 24 commits into from
Sep 1, 2023
Merged

Initial plants model draft #296

merged 24 commits into from
Sep 1, 2023

Conversation

davidorme
Copy link
Collaborator

Description

This PR provides the first steps in setting up the plants model. The functionality in this PR is purely focussed on the initial setup of the plant community:

  • Classes are defined to define plant functional types and generate those from the configuration.
  • A Plants class is defined to hold lists of PlantCohorts by grid cell and to populate that from provided init data, which is basically just a data frame of plant cohort details.

The next steps for future PRs are to:

  1. generate canopy model from the community data and insert canopy leaf area index and height into the data object.
  2. implement an update method to estimate productivity, allocation to growth and hence a new canopy model, but this is not likely to happen soon.
  3. Work out how to modify leaf area index in response to herbivory.

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

Copy link
Collaborator

@jacobcook1995 jacobcook1995 left a comment

Choose a reason for hiding this comment

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

LGTM! I just had a few minor comments

Copy link
Collaborator

@vgro vgro 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 overall very good, I added a few minor comments.
It would be useful if you could give your variables longer, more descriptive names. Please add tests for the plant_model.

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2023

Codecov Report

Merging #296 (9c2e25f) into develop (bd86d67) will decrease coverage by 0.53%.
The diff coverage is 88.09%.

@@             Coverage Diff             @@
##           develop     #296      +/-   ##
===========================================
- Coverage    97.55%   97.03%   -0.53%     
===========================================
  Files           47       51       +4     
  Lines         2171     2293     +122     
===========================================
+ Hits          2118     2225     +107     
- Misses          53       68      +15     
Files Changed Coverage Δ
...rtual_rainforest/models/plants/functional_types.py 67.56% <67.56%> (ø)
virtual_rainforest/models/plants/plants_model.py 91.89% <91.89%> (ø)
virtual_rainforest/models/plants/__init__.py 100.00% <100.00%> (ø)
virtual_rainforest/models/plants/community.py 100.00% <100.00%> (ø)
virtual_rainforest/models/plants/constants.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@davidorme
Copy link
Collaborator Author

@vgro I was holding off on the PlantModel tests until there was something to test, but actually even the minimal tests have caught a couple of issues 😄 Can you have a quick double check of the requested changes?

@davidorme davidorme requested a review from vgro August 29, 2023 13:48
Copy link
Collaborator

@vgro vgro left a comment

Choose a reason for hiding this comment

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

LGTM! A few minor comments :-)

@vgro
Copy link
Collaborator

vgro commented Aug 29, 2023

@vgro I was holding off on the PlantModel tests until there was something to test, but actually even the minimal tests have caught a couple of issues 😄

Haha, yes, annoyingly (and luckily) it always does ;-)

@davidorme davidorme merged commit 1e0ca4e into develop Sep 1, 2023
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.

4 participants