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

abiotic_tools introduced #186

Merged
merged 4 commits into from
Feb 28, 2023
Merged

abiotic_tools introduced #186

merged 4 commits into from
Feb 28, 2023

Conversation

vgro
Copy link
Collaborator

@vgro vgro commented Feb 28, 2023

I found a number of functions and universal constants that will be used by multiple submodules of the abiotic_model.
I introduced a new submodule called abiotic_tools which contains them all and can be accessed easily by other submodules.

The AbioticConstants might eventually go to a central point or can be accessed by others, for now it just makes the abiotic part of the model easier to manage.

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

@@ -0,0 +1,54 @@
"""Test abiotic_tools.py."""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tests are cut and paste from the wind submodule, so they already went through PR process.

@vgro vgro marked this pull request as draft February 28, 2023 08:42
@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2023

Codecov Report

Merging #186 (c670fdf) into develop (c7c7724) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #186      +/-   ##
===========================================
+ Coverage    95.51%   95.53%   +0.02%     
===========================================
  Files           24       25       +1     
  Lines         1181     1187       +6     
===========================================
+ Hits          1128     1134       +6     
  Misses          53       53              
Impacted Files Coverage Δ
virtual_rainforest/models/abiotic/__init__.py 100.00% <ø> (ø)
virtual_rainforest/models/abiotic/abiotic_tools.py 100.00% <100.00%> (ø)
virtual_rainforest/models/abiotic/radiation.py 89.28% <100.00%> (-0.19%) ⬇️
virtual_rainforest/models/abiotic/wind.py 100.00% <100.00%> (ø)

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

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.

This seems like a sensible idea, I noticed a typo, but otherwise looks good to me!

@vgro vgro marked this pull request as ready for review February 28, 2023 08:46
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.

The only thing I'd query here is that abiotic_tools is a bit vague. You basically have functions and constants. I could definitely see us having a standard structure within models of having model.constants and model.functions.

I don't think that should stop this PR - the aim of centralising those objects is fine - but we might revisit how these are arranged. For example, one thing might be that all constants classes typically go in a file (Wind and Radiation going with the general ones). There's also a case for them staying with their classes though.

Comment on lines +1 to +2
"""The ``models.abiotic.abiotic_tools`` module contains a set of general functions and
universal constants that are shared across submodels in the abiotic model.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this deliberately not a link? It might well be, because it would be a link to itself, basically, but just checking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I followed the syntax from other modules here. I could add a link to the abiotic_model, but I think that can wait until I had a think about what else to write in the docstring that is a bit more descriptive. For know it's just to continue with other things more easily.

We should definitely have a general discussion about constants and where they should go, at the moment each abiotic submodule has a data class at the top but this is not meant to stay there, it could well go in an abiotic.constants file that constains all data classes.

@vgro vgro merged commit 9bfa485 into develop Feb 28, 2023
@vgro vgro deleted the feature/abiotic_tools branch February 28, 2023 09:00
@vgro
Copy link
Collaborator Author

vgro commented Feb 28, 2023

Further discussion on uniform structure of constants needed, see #129 and #186 (review)

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