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

Update traits data structures #300

Merged
merged 6 commits into from
Sep 28, 2024
Merged

Conversation

davidorme
Copy link
Collaborator

Description

This PR implements the updated trait data structures requested in #298. It update tests and things using the trait data structures.

It forms part of a sequence re-implementing the functionality set implemented in #296 but on less shaky structural grounds and in smaller more easily reviewed sections.

Fixes #298 (issue)

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

@davidorme davidorme linked an issue Sep 27, 2024 that may be closed by this pull request

@dataclass()
class StemTraits:
"""A dataclass for stem traits.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to make Flora inherit StemTraits, or have both inherit from an ABC, so that this list of traits is only defined in one place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ha. I did look at all sorts of things here - Protocols and ABC and inheritance, but these classes are basically intended to deal with two different constructor signatures, even if they share nearly all the attributes. That seems to make any kind of inheritance more finicky than just duplicating stuff. I may have missed it - I'd love a way to share that set of attributes cleanly across the two PFT traits, Flora and Stem Traits.

Right now though, I'd like to define that common API on the attributes and get the next PR in to close out the functionality hanging in #296 - and if the API looks good, I'm all for circling back to improve the implementation later.

@j-emberton j-emberton self-requested a review September 28, 2024 15:25
Copy link
Collaborator

@j-emberton j-emberton left a comment

Choose a reason for hiding this comment

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

Happy to approve to keep the demography work stream moving. I'm wary of accumulating technical debt (i.e. not coming up with an improved implementation of the stem traits that avoids duplication) but appreciate we need to get the remaining stuff in as well.

@davidorme davidorme merged commit 4c347a1 into develop Sep 28, 2024
12 checks passed
@davidorme davidorme deleted the 298-update-traits-data-structures branch September 28, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update traits data structures
2 participants