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

Use xsdba #530

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Use xsdba #530

wants to merge 22 commits into from

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Feb 11, 2025

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • (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.
  • CHANGELOG.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?

  • xscen directly import xsdba

Does this PR introduce a breaking change?

xclim_train_args -> xsdba_train_args and so on, so yes.

Other information:

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 the docs label Feb 11, 2025
tests/test_biasadjust.py Outdated Show resolved Hide resolved
Copy link
Contributor

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

Explicitly requesting changes so that we don't forget to replace the xsdba dependency with a stable version.

@@ -63,6 +63,7 @@ dependencies = [
"toolz",
"xarray >=2023.11.0, !=2024.6.0, <2024.10.0", # FIXME: 2024.10.0 breaks rechunker with zarr
"xclim >=0.53.2, <0.55",
"xsdba@git+https://[email protected]/Ouranosinc/xsdba.git@naming_conventions",
Copy link
Contributor

@Zeitsperre Zeitsperre Feb 11, 2025

Choose a reason for hiding this comment

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

FYI, this will not pass the PyPI smell test. Pointing to git branches for dependencies is forbidden on PyPI (thankfully).

We can release a patch (for xsdba) when the branch (on xsdba) is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just have a new xsdba release before this PR is merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Exactly that.

docs/notebooks/2_getting_started.ipynb Outdated Show resolved Hide resolved
docs/notebooks/2_getting_started.ipynb Outdated Show resolved Hide resolved
docs/notebooks/2_getting_started.ipynb Outdated Show resolved Hide resolved
src/xscen/biasadjust.py Outdated Show resolved Hide resolved
tests/test_biasadjust.py Outdated Show resolved Hide resolved
Comment on lines 63 to 64
group: sdba.Grouper | str | dict | None = None,
xclim_train_args: dict | None = None,
group: xsdba.Grouper | str | dict | None = None,
xsdba_train_args: dict | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be a breaking change.

We should instead support both options for a while, but issue a FutureWarning if someone uses xclim_train/adjust_args. The rest of the code can use the new names, just with xsdba_train/adjust_args = xclim_train/adjust_args for those cases.

@coxipi
Copy link
Contributor Author

coxipi commented Feb 18, 2025

In extract.py::resample, there's an issue with the new xclim, presumably following the recent PR about missing method

  >           complete = ~xc.core.missing.MISSING_METHODS[missmeth](
                  da, target_frequency, initial_frequency
              )(**missing)

@aulemahal
Copy link
Collaborator

@coxipi Je m'en occupe, c'est ma faute!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants