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

Reintroduce PercentileDataArray to fix older icclims #1294

Closed
2 tasks done
aulemahal opened this issue Feb 14, 2023 · 6 comments
Closed
2 tasks done

Reintroduce PercentileDataArray to fix older icclims #1294

aulemahal opened this issue Feb 14, 2023 · 6 comments
Assignees
Labels
bug Something isn't working priority Immediate priority

Comments

@aulemahal
Copy link
Collaborator

Setup Information

  • Xclim version: 0.40
  • Python version: all
  • Operating System: all

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

  • I would be willing/able to open a Pull Request to address this bug.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@aulemahal aulemahal added the bug Something isn't working label Feb 14, 2023
@aulemahal aulemahal self-assigned this Feb 14, 2023
@aulemahal aulemahal added the priority Immediate priority label Feb 14, 2023
@bzah
Copy link
Contributor

bzah commented Feb 14, 2023

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
Copy link
Collaborator Author

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
Copy link
Contributor

bzah commented Feb 14, 2023

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
Copy link
Collaborator Author

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
Copy link
Contributor

bzah commented Feb 15, 2023

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
Copy link
Contributor

bzah commented Feb 20, 2023

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

@bzah bzah closed this as completed Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority Immediate priority
Projects
None yet
Development

No branches or pull requests

2 participants