-
Notifications
You must be signed in to change notification settings - Fork 1
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
adding unit and name attributes to layers #36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add the required attributes to the total as well as the layers in the sum_layers
function.
We should also add a test to the test_om_prior.py
which verifies that the units are in the output. This helps to ensure that they don't get lost if we do further changes. This one is a bit annoying because it requires running the whole workflow to test this so it can be a bit slow.
A simple test like this could be sufficient for now:
def test_required_attributes(output_domain_xr):
assert output_domain_xr.variables["OCH4_TOTAL"].attrs == {
"units": "kg/m^2/s",
"long_name": "OCH4_TOTAL",
}
assert output_domain_xr.variables["OCH4_WETLANDS"].attrs == {
"units": "kg/m^2/s",
"long_name": "OCH4_WETLANDS",
}
This currently fails until the fix to the total has been implemented.
We should have a session on how to run pytest and the test suite.
Addresses #16
added the attributes, I'll leave the test to someone who understands pytest
|
@prayner Are you able to see any line-specific comments or are they not available in emacs? |
Don't think I can see them in emacs ... let's see if the gh cli
reveals them
|
I'm not seeing in-line comments, are there some here?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok good to know.
For constants, "SCREAMING_SNAKE_CASE" is the preference. If we don't change this as part of this PR it will likely later be cleaned up.
I had another comment to use setncatts
to set all the values at the same time instead of looping.
I'll fix them up with the tests
@prayner Can I add the tests to this branch? If so please assign this PR to me |
* main: (41 commits) chore: Copy .env file chore: Include dev and tests chore: Include dev and tests chore: Include dev and tests refactor: Move reproject_raster_inputs to raster chore: remove poetry chore: Fix image chore: vendor the reproject code chore: Try avoid blowing up the cache size by only caching the final image chore: Simplify chore: Try again chore: Try again chore: Use venv pip chore: ls chore: Another attempt 5 chore: Another attempt 4 chore: Another attempt 3 chore: Another attempt 2 chore: Another attempt chore: testing ...
addresses #16
Description
Checklist
Please confirm that this pull request has done the following:
Notes