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

Accept float in days_over_precip_thresh #258

Merged
merged 6 commits into from
Sep 16, 2019
Merged

Accept float in days_over_precip_thresh #258

merged 6 commits into from
Sep 16, 2019

Conversation

huard
Copy link
Collaborator

@huard huard commented Aug 21, 2019

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated

After merging to master and before closing the branch:

  • bumpversion (minor / major / patch) has been called on master branch
  • Tags have been pushed
  • What kind of change does this PR introduce?

  • Add support for simple annual percentiles.

  • Does this PR introduce a breaking change?
    No

  • Other information:
    TODO

  • Review code and naming.
  • Apply same logic to other relevant functions

This PR fixes #79

@coveralls
Copy link

coveralls commented Aug 21, 2019

Pull Request Test Coverage Report for Build 1094

  • 11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
  • 19 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.5%) to 92.989%

Files with Coverage Reduction New Missed Lines %
xclim/indices/_multivariate.py 1 99.35%
xclim/indices/_simple.py 2 97.65%
xclim/utils.py 16 95.03%
Totals Coverage Status
Change from base Build 1041: 0.5%
Covered Lines: 1313
Relevant Lines: 1412

💛 - Coveralls

huard and others added 3 commits August 21, 2019 15:50
Co-Authored-By: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
@huard
Copy link
Collaborator Author

huard commented Aug 22, 2019

Is the name resample_doy sufficiently informative ? resample_from_doy ?

@huard
Copy link
Collaborator Author

huard commented Aug 23, 2019

@Zeitsperre Are the azure failures expected ? Is this something we really want to invest in ?

@Zeitsperre
Copy link
Collaborator

@huard Yeah, I'm thinking it might be worthwhile to move from Azure as well. The goal was simply to perform the same sort of platform checks as macOS. I learned a few weeks ago that Travis CI recently added a Windows image. I'd much rather use their infra.

I'll open an issue for replacing Azure with Travis CI for Windows builds.

@huard
Copy link
Collaborator Author

huard commented Sep 6, 2019

@tlogan2000 Could you take a look ?

@Zeitsperre
Copy link
Collaborator

@huard The Azure builds are no longer valid. There was a misconfiguration in their .yaml and I didn't see the value in investing time into making them all work. I feel like we can wait for Travis CI to create some Windows images.

For this PR, I would simply ignore those checks. Everything is passing on Travis.

@tlogan2000
Copy link
Collaborator

@tlogan2000 Could you take a look ?

I will take a look at this soon... sorry it slipped through a crack I think

@huard
Copy link
Collaborator Author

huard commented Sep 12, 2019

@tlogan2000 Removed support for str arguments, thanks for catching this.

Found a small bug in xarray while writing new tests. A PR is submitted pydata/xarray#3305

@tlogan2000
Copy link
Collaborator

@huard with the roll back of float/string input what new features does this PR still include? Is it the doy_resample()?

@huard
Copy link
Collaborator Author

huard commented Sep 13, 2019

Yes, that's mostly it. I'd call it a clean-up of how doy percentiles are handled.

Temperature indicators require a doy percentile, while precip percentiles also accept a single "annual" value.

@tlogan2000
Copy link
Collaborator

Would an example of using an annual percentile DataArray (i.e. not using percentile_doy()) be useful
Currently the help shows :

p75 = percentile_doy(historical_pr, per=0.75)
r75p = days_over_precip_thresh(pr, p75)

but users could also send in a 2d array of the annual percentile (NB* not sure of my syntax here)

p75ann = historical_pr.quantile(dim-'time',q=0.75)

@tlogan2000
Copy link
Collaborator

Yes, that's mostly it. I'd call it a clean-up of how doy percentiles are handled.

Temperature indicators require a doy percentile, while precip percentiles also accept a single "annual" value.

Sounds good

@huard
Copy link
Collaborator Author

huard commented Sep 13, 2019

Made change in docstring. Won't really work as-is however until xarray 0.13 because keep_attrs does not yet work for DataArray.quantiles.

@huard huard merged commit 3fbf7a4 into master Sep 16, 2019
@huard huard deleted the fix-79 branch September 16, 2019 20:45
@Zeitsperre
Copy link
Collaborator

@huard if you want to update the history and bumpversion patch master, I can pull the new master and send it to PyPI/conda-forge.

@huard
Copy link
Collaborator Author

huard commented Sep 16, 2019

@Zeitsperre
So bumpversion does not actually change the tag anymore ? I had to commit manually the change and call git tag myself... weird.

@Zeitsperre
Copy link
Collaborator

The tagging is re-enabled on another PR since I realize it doesn't cause commits to fail (only commit = true seems to do that). I'll inform people when that is in master.

I had it turned off since it was conflicting with pre-commit. It turns out that the bumpversion python library has a few bugs with automatically adding lines (causing code formatting to fail). This might be fixed in a fork that Birdhouse uses called bump2version but I need to look into it.

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.

Percentile threshold indices : Frequencies?
4 participants