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.

Sign up for GitHub

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

Jump to bottom

xclim 0.48 compatibility #351

Merged
merged 33 commits into from
Feb 27, 2024
Merged

xclim 0.48 compatibility #351

merged 33 commits into from
Feb 27, 2024

Conversation

RondeauG
Copy link
Collaborator

@RondeauG RondeauG commented Feb 21, 2024 •
edited
Loading

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • No
  • (If applicable) Documentation has been added / updated (for bug fixes / features).
  • (If applicable) Tests have been added.
  • This PR does not seem to break the templates.
  • CHANGES.rst has been updated (with summary of main changes).
    • Link to issue (:issue:number) and pull request (:pull:number) has been added.
  • Maybe... Add a workaround to convert the old xrfreq nomenclature to the new one upon loading an existing catalog or parsing a directory.

What kind of change does this PR introduce?

  • Changes to be compatible with recent versions of pandas/xarray/xclim

Does this PR introduce a breaking change?

  • xscen now requires pandas >= 2.2
  • Functions that output a dict with keys as xrfreq (extract_dataset, compute_indicators) will now return the new nomenclature.
  • The frequency --> xrfreq/timedelta will now only result in the new nomenclature for hours (lower case h).

Other information:

Zeitsperre
Zeitsperre reviewed Feb 21, 2024
View reviewed changes
environment-dev.yml Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@RondeauG
Copy link
Collaborator Author

Let's wait for Ouranosinc/xclim#1664 before we finish this PR.

@RondeauG RondeauG mentioned this pull request Feb 22, 2024
Merged
5 tasks
@Zeitsperre
Copy link
Contributor

Zeitsperre commented Feb 22, 2024 •
edited
Loading

The problem with Python3.12 is quite silly. Pushing a fix in a few mins.

@aulemahal
Copy link
Collaborator

I can push something that ensures the new xrfreq is used when we open catalogs (and parse them).

@aulemahal
Copy link
Collaborator

Last commit added a ensure_new_xrfreq function to utils. It is used when opening a catalog and in parse_directory. This means that legacy frequencies either in the paths or in the catalogs shouldn't be a problem.

Running on MRCC5-disponible, our largest catalog, my n=1 test finds that the opening time has increased by 6%. I decided that this was small enough not to require further optimization or skipping options.

@Zeitsperre
Copy link
Contributor

I'll be releasing xclim v0.48.2 today!

@Zeitsperre
Copy link
Contributor

This PR is waiting on the newest patch release from conda-forge. Should be available later today.

@RondeauG
Copy link
Collaborator Author

xclim>=0.48.0 ou xclim>=0.48.2 ?

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

juliettelavoie
juliettelavoie reviewed Feb 27, 2024
View reviewed changes
tox.ini
@@ -69,6 +69,7 @@ download = true
deps =
coveralls: coveralls
esmpy: git+https://github.com/esmf-org/esmf.git@v{env:ESMF_VERSION}\#subdirectory=src/addon/esmpy
xclim: git+https://github.com/Ouranosinc/xclim.git@main
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still necessary with the new release ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I figured we could leave it there and have at least one build that checks against the main branch. For xclim, we have a dedicated upstream build; Would it be prudent to implement that here as well? (in another PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. no strong opinions from me for the upstream build.

Copy link
Contributor

Choose a reason for hiding this comment

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

You say "no strong opinions", I read "no opposition, go for it". You got it. ;)

aulemahal
aulemahal approved these changes Feb 27, 2024
View reviewed changes
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

One fix, but the rest seems to be good!

xscen/CVs/xrfreq_to_timedelta.json Outdated Show resolved Hide resolved
juliettelavoie
juliettelavoie reviewed Feb 27, 2024
View reviewed changes
xscen/catutils.py Outdated Show resolved Hide resolved
juliettelavoie
juliettelavoie approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

lgtm

@RondeauG RondeauG merged commit 90151f3 into main Feb 27, 2024
24 checks passed
@RondeauG RondeauG deleted the compatxclim48 branch February 27, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zeitsperre Zeitsperre Zeitsperre left review comments

@aulemahal aulemahal aulemahal approved these changes

@juliettelavoie juliettelavoie juliettelavoie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RondeauG @Zeitsperre @aulemahal @juliettelavoie