search

Ouranosinc / xclim

Library of derived climate variables, ie climate indicators, based on xarray.
https://xclim.readthedocs.io/en/stable/
Apache License 2.0
333 stars 59 forks source link

Reintroduce PercentileDataArray to fix older icclims #1294

Closed aulemahal closed 1 year ago

aulemahal commented 1 year ago

Setup Information

Description

1236 broke icclim by removing the PercentileDataArray class. cerfacs-globc/icclim#251 did fix icclim, but there was no release since and installing icclim is still broken.

Steps To Reproduce

pip install icclim
python -c "import icclim"

Additional context

Xclim will make no use of the class, but at least older icclim will still work.

Contribution

Code of Conduct

bzah commented 1 year ago

Is it a temporary fix for a few days or do you intent to keep this for a while ?

Just my opinion, but I think this should be addressed on icclim not on xclim. We (icc) didn't strongly pin xclim version in the past versions of icclim, it's unfortunate because now all icclim's published version on pipy and conda-forge are unusable. However, I think until xclim reaches a 1.0.0, xclim's "contract of use" should be something like "there will be breaking changes between minor versions, you must be able to deal with it."

I would suggest to patch icclim latest with a pin to xclim==0.39 to fix this issue (I'm working on it).

aulemahal commented 1 year ago

It is to be a temporary fix indeed. We had a discussion this morning with Christian and this seemed a good short-term fix.

The thing is, we're not 100% sure we will ever have a version 1.0. Packages around us are not following this scheme (xarray, dask, etc) and we fear the maintenance and development burden might be too heavy?

If icclim depends so strongly on xclim, indeed a good method would be to pin it. We do this for finch, for example.

bzah commented 1 year ago

The 0ver versioning strategy!

Jokes aside, I understand how tempting it is, but to limit burden on maintenance you can clearly state in xclim documentation that only latest major version is maintained no ?

If icclim depends so strongly on xclim, indeed a good method would be to pin it. [...]

Yeah I should have done that for the past versions, I didn't realize it could lead to this situation. It will be fixed in icclim 6.2.0.

aulemahal commented 1 year ago

Haha, I think we were mainly thinking of the Calendar versioning strategy ;)

We haven't thought of this much, but IMO the maintenance burden is more in all the workarounds one would do to avoid breaking xclim in a minor version update, when it is not "worth" a major update.

bzah commented 1 year ago

I personally prefer semver, but I would be happy to see xclim adopt calver !

We haven't thought of this much, but IMO the maintenance burden is more in all the workarounds one would do to avoid breaking xclim in a minor version update, when it is not "worth" a major update.

I agree, I had the issue in icc and I updated the minor even when there was some (minor) breaking changes a few times ... I think we should normalize incrementing the major number though.

As for the original issue, icclim 6.2 is available on pipy and will be very soon on conda-forge, so this issue can be closed IMO, ping @pagecp.

bzah commented 1 year ago

Closing as this was fixed on icclim. Feel free to reopen if needed.