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

Add partial support for L-Moments #526

Merged
merged 13 commits into from
Aug 20, 2020
Merged

Add partial support for L-Moments #526

merged 13 commits into from
Aug 20, 2020

Conversation

huard
Copy link
Collaborator

@huard huard commented Aug 17, 2020

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)
  • Documentation has been added / updated (for bug fixes / features)
  • HISTORY.rst has been updated (with summary of main changes)
  • bumpversion (minor / major / patch) has been called on this branch
  • Tags have been pushed (git push --tags)
  • What kind of change does this PR introduce?
    Adds a pwm_fit method, estimating statistical distribution parameters from a sample using probability weighted moments.
    Only a few distributions are supported.

  • Does this PR introduce a breaking change?
    No.

  • Other information:
    Requires the lmoments3 library, but it is unmaintained anymore. It's not clear if we should add it as a dependency or work on a more robust solution. Needs to be installed from master because the last release is not compatible with recent scipy releases.

@coveralls
Copy link

coveralls commented Aug 17, 2020

Pull Request Test Coverage Report for Build 2191

  • 17 of 18 (94.44%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.005%) to 91.77%

Changes Missing Coverage Covered Lines Changed/Added Lines %
xclim/indices/generic.py 17 18 94.44%
Files with Coverage Reduction New Missed Lines %
xclim/indices/generic.py 1 90.59%
Totals Coverage Status
Change from base Build 2182: 0.005%
Covered Lines: 2899
Relevant Lines: 3159

💛 - Coveralls

xclim/indices/generic.py Outdated Show resolved Hide resolved
@huard
Copy link
Collaborator Author

huard commented Aug 19, 2020

@Zeitsperre How do you suggest we deal with the lmoments3 dependency? Since it is unmaintained, I'm not sure it's a good idea to include this "officially" in the requirements, but we probably still want to run it in the travis test suite.

@Zeitsperre
Copy link
Collaborator

Fully agree that adding unmaintained libraries to the requirements is bad form. Plenty of ways forward though.

There's a couple of ways we could deal with it from the CI side:

  • Add a new CI build for it (expensive on resources)
  • Retrofit an existing CI build for it (like the py36-nosubset build)

In terms of how to proceed requirements-wise, we can treat functions reliant on it the same way we treat plotting functions, (IE creating a global variable like LM3_INSTALLED in the module that uses it, aborting functions that need it when it isn't there).

If we want users to be able to run tests locally with lmoments3 installed, we would put the dependency in the tox.ini, and if we only want to run it on Travis CI, we can add it to the .travis.yml.

@Zeitsperre
Copy link
Collaborator

Whichever way forward you want to try, let me know and I can push some changes here.

@huard
Copy link
Collaborator Author

huard commented Aug 19, 2020

@Zeitsperre I suggest we go with tox.ini to allow local testing. Ok to bundle the dependency with another suite. We could have a test environment dedicated to all optional external dependencies like roocs. Note that the lmoments3 library has to be installed from master.
pip install git+https://github.com/OpenHydrology/lmoments3.git

@huard huard requested a review from aulemahal August 19, 2020 21:48
@huard
Copy link
Collaborator Author

huard commented Aug 19, 2020

@aulemahal Combined fit and fit_pwm into one single function.

xclim/indices/generic.py Outdated Show resolved Hide resolved
Co-authored-by: Pascal Bourgault <[email protected]>
@aulemahal
Copy link
Collaborator

Woupelaye. The build failures are due to OpenHydrology/lmoments3#6, which is open and inactive since june 2019...

@huard
Copy link
Collaborator Author

huard commented Aug 20, 2020

Ah right, it's the develop branch.

xclim/indices/generic.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@huard huard left a comment

Choose a reason for hiding this comment

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

change comment to use develop branch.

@Zeitsperre
Copy link
Collaborator

Added in the formatting exceptions. Having the import in a function with no space afterwards breaks two hooks, haha.

@huard huard merged commit 846f0bc into master Aug 20, 2020
@huard huard deleted the fix-33 branch August 20, 2020 17:09
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.

Fit distributions using L-moments in addition to ML
4 participants