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

Remove lmoments3 #1644

Merged
merged 11 commits into from
Feb 16, 2024
Merged

Remove lmoments3 #1644

merged 11 commits into from
Feb 16, 2024

Conversation

aulemahal
Copy link
Collaborator

@aulemahal aulemahal commented Feb 13, 2024

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)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGES.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • Remove lmoments3 from xclim's dependencies.
  • Accept rv_continuous instances for the dist arguments of the statistical indices

Does this PR introduce a breaking change?

Yes. Passing a mere string and method='PWM' is now broken. It will raise a ValueError with a message asking to pass an instance of lmoments3 instead.

This also makes some functions awkward to use. Before, we relied on the scipy_dist attribute of params to retrieve the distribution when computing statistics. As one can now pass an object that has nothing to do with scipy, those functions (parametric_quantile for example) must now also accept the dist as an argument.

Other information:

See Ouranosinc/lmoments3#12.

I kinda cheated and made it so the dist argument would show up as a "String" parameter to the indicator. This only changes the metadata, distributions objects can still be passed to indicators.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added CI Automation and Contiunous Integration docs Improvements to documenation labels Feb 13, 2024
Copy link

Note
It appears that this Pull Request modifies the main.yml workflow.

On inspection, the XCLIM_TESTDATA_BRANCH environment variable is set to the most recent tag (v2023.12.14).

No further action is required.

Copy link
Collaborator

@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.

Looks good. Could you confirm stats.frequency_analysis still works ?

CHANGES.rst Outdated Show resolved Hide resolved
xclim/indices/stats.py Outdated Show resolved Hide resolved
tests/test_stats.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Feb 14, 2024
@aulemahal
Copy link
Collaborator Author

@huard
This code works:

import xclim as xc
import xarray as xr
import numpy as np
import lmoments3.distr as lmom

q = xr.DataArray(
    np.random.random_sample((365 * 10 + 2,)), 
    dims=('time',),
    coords={'time': xr.cftime_range('1990-01-01', freq='D', periods=365 * 10 + 2)},
    attrs={'units': 'm3 s-1'}
)

xc.generic.return_level(q, mode='max', t=2, dist=lmom.gev, window=6, freq='YS', method='PWM')
xc.generic.return_level(q, mode='max', t=2, dist='genextreme', window=6, freq='YS', method='ML')

@aulemahal
Copy link
Collaborator Author

@saschahofmann If you did not receive news from the last person, does this PR seems reasonable to you ?

@coveralls
Copy link

coveralls commented Feb 14, 2024

Coverage Status

coverage: 90.164% (+0.004%) from 90.16%
when pulling 4a51936 on remove-lmoments
into d09bd4b on master.

@saschahofmann
Copy link
Contributor

Yes totally . Tomorrow I'll be a little rude and give him a nudge on the lmoments R library that he still maintains hoping we can get his attention that way

Copy link
Collaborator

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

Nicely done!

CHANGES.rst Outdated Show resolved Hide resolved
docs/notebooks/frequency_analysis.ipynb Outdated Show resolved Hide resolved
xclim/indices/stats.py Outdated Show resolved Hide resolved
@Zeitsperre Zeitsperre added the priority Immediate priority label Feb 14, 2024
@Zeitsperre Zeitsperre merged commit 1385fca into master Feb 16, 2024
21 checks passed
@Zeitsperre Zeitsperre deleted the remove-lmoments branch February 16, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests CI Automation and Contiunous Integration docs Improvements to documenation priority Immediate priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration of lmoments3 and eofs is not permitted under GPLv3
5 participants