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

Allowing time_utils.select_time_period to work on arrays containing NaNs #12

Conversation

dougiesquire
Copy link
Collaborator

@dougiesquire dougiesquire commented Dec 6, 2021

This PR extends time_utils.select_time_period to work on arrays containing NaNs.

Note this builds off the branch requested for merge in #9, so best to merge that first

Closes #11

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #12 (7487854) into master (b38788e) will increase coverage by 1.70%.
The diff coverage is 93.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
+ Coverage   16.52%   18.23%   +1.70%     
==========================================
  Files          11       15       +4     
  Lines         599      894     +295     
==========================================
+ Hits           99      163      +64     
- Misses        500      731     +231     
Impacted Files Coverage Δ
unseen/array_handling.py 41.79% <ø> (ø)
unseen/tests/conftest.py 91.89% <91.89%> (ø)
unseen/time_utils.py 33.89% <92.30%> (+18.85%) ⬆️
unseen/tests/test_time_utils.py 95.00% <95.00%> (ø)
unseen/tests/test_array_handling.py 100.00% <100.00%> (+3.70%) ⬆️
unseen/fileio.py 0.00% <0.00%> (ø)
unseen/indices.py 0.00% <0.00%> (ø)
unseen/bias_correction.py 0.00% <0.00%> (ø)
unseen/independence.py 0.00% <0.00%> (ø)
unseen/similarity.py 0.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b38788e...7487854. Read the comment docs.

Copy link
Member

@DamienIrving DamienIrving left a comment

Choose a reason for hiding this comment

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

Thanks for adding this functionality, @dougiesquire.

Before merging we just need to decide on a few general principles that apply to many/all functions we write:

  • Google or Numpy style docstrings?
  • Assumed dimension names or keyword argument defaults?
  • Dual xarray Dataset/DataArray functionality (and thus whether to use ds or da as a generic variable name)

@dougiesquire
Copy link
Collaborator Author

Thanks for the review Damien. I've implemented what we discussed in person:

  • Use numpy-style doc strings
  • Rename da to ds
  • Include options args "time_name" etc and specify in the docstring whether these are dims, coords, variables or can be multiple
  • Test functions using both DataArrays and Datasets

I'll go ahead and merge if you have no issues

Copy link
Member

@DamienIrving DamienIrving left a comment

Choose a reason for hiding this comment

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

Thanks, @dougiesquire. This looks great.

@dougiesquire dougiesquire merged commit 77be826 into AusClimateService:master Dec 13, 2021
@dougiesquire dougiesquire deleted the skip_nan_in_select_time_period branch December 13, 2021 22:59
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.

Allow time_utils.select_time_period to work on arrays containing NaNs
3 participants