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

Temporarily revert star annotation in call signatures - remove all internal xarray imports #2116

Merged
merged 20 commits into from
Mar 25, 2025

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Mar 21, 2025

Pull Request Checklist:

What kind of change does this PR introduce?

  • Reverts star-annotated call signatures (not supported for Python 3.10)
    *xclim now uses directly operator instead of using xarray's derived get_op function. A refactoring in xarray had changed the position of get_op which caused a bug.
    • All other uses of xarray's internal API were also removed (:pull:2116).
  • Updates the pre-commit hooks.

Does this PR introduce a breaking change?

No.

@Zeitsperre Zeitsperre added the bug Something isn't working label Mar 21, 2025
@Zeitsperre Zeitsperre requested a review from aulemahal March 21, 2025 16:48
@Zeitsperre Zeitsperre self-assigned this Mar 21, 2025
@github-actions github-actions bot added the CI Automation and Contiunous Integration label Mar 21, 2025
@github-actions github-actions bot added the approved Approved for additional tests label Mar 21, 2025
@Zeitsperre
Copy link
Collaborator Author

This PR needs the changes from #2114

@Zeitsperre Zeitsperre changed the title Tmporarily revert star annotation in call signatures Temporarily revert star annotation in call signatures Mar 21, 2025
…2114)

### What kind of change does this PR introduce?

* This PR fixes an issue with an operation not being available in xarray
versions later than 2025.3.0

### Does this PR introduce a breaking change?

No breaking changes
@github-actions github-actions bot added the docs Improvements to documenation label Mar 24, 2025
Copy link

github-actions bot commented Mar 24, 2025

Note

This Pull Request modifies the SDBA module.
The changes made here have been approved. Be sure to port any relevant changes to xsdba.
[!NOTE]
This Pull Request modifies the SDBA module.
The changes made here have been approved. Be sure to port any relevant changes to xsdba.
[!NOTE]
This Pull Request modifies the SDBA module.
The changes made here have been approved. Be sure to port any relevant changes to xsdba.
[!NOTE]
This Pull Request modifies the SDBA module.
The changes made here have been approved. Be sure to port any relevant changes to xsdba.
[!NOTE]
This Pull Request modifies the SDBA module.
The changes made here have been approved. Be sure to port any relevant changes to xsdba.
[!NOTE]
This Pull Request modifies the SDBA module.
The changes made here have been approved. Be sure to port any relevant changes to xsdba.

@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Mar 24, 2025
@aulemahal aulemahal changed the title Temporarily revert star annotation in call signatures Temporarily revert star annotation in call signatures - remove all internal xarray imports Mar 24, 2025
@aulemahal
Copy link
Collaborator

aulemahal commented Mar 24, 2025

@Zeitsperre My last commit removed all xarray imports that were not from their "public" API, hoping that avoids all further problemes like we add with xarray 2025.3.

To do so, I had to copy a few lines from xarray for the merge_attrs_drop_conflicts and get_temp_dimname functions. @coxipi you might want to the the same as I did for xsdba, even though this is not a bug right now, just futurproofing.

I also had to remove DataArrayResample and DatasetResample from the signature of xc.core.calendar.time_bnds. They are still mentionned in the docstring... Not the best, but I don't see how I can have them in the signature without importing them at runtime....

I also fixed an issue with setting options that are deep dictionaries. And fixed the issue with the missing methods. Flox is now enabled by default for resampling with first and last, but the way xarray calls flox is broken for datetime data in some cases. I simply used another resampling method as the line in expected_count only cares about the output coordinates. (This call is done because the alternative was to dig into private xarray objects)

@Zeitsperre
Copy link
Collaborator Author

I also fixed an issue with setting options that are deep dictionaries. And fixed the issue with the missing methods. Flox is now enabled by default for resampling with first and last, but the way xarray calls flox is broken for datetime data in some cases. I simply used another resampling method as the line in expected_count only cares about the output coordinates. (This call is done because the alternative was to dig into private xarray objects)

Thanks for the fixes! Think you could mention this in the Changelog as a (potentially) breaking change?

@coveralls
Copy link

coveralls commented Mar 24, 2025

Coverage Status

coverage: 89.96% (-0.04%) from 90.004%
when pulling 668f83d on revert-star-annotation
into bade70a on main.

@github-actions github-actions bot added the indicators Climate indices and indicators label Mar 24, 2025
@aulemahal
Copy link
Collaborator

as a (potentially) breaking change?

I don't think the user is impacted at all! This change does not change anything in the function's outputs.

I found more xarray internals and removed them. I had to reimplement a version of days_in_year to do so, but the version I copied from xarray is pretty short.

The last xarray internal calls I see, but couldn't/wouldn't remove are:

  • xr.core.common._contains_cftime_datetimes and xr.conventions.encode_cf_variable in xclim.sdba.map_blocks : I think the whole block of code can be removed, but I will do so in a PR to xsdba instead.
  • xr.coding.cftime_offsets.to_offset in a few places : we need the returned Offset object, which itself isn't public API anyway. No way I am reimplementing this whole machinery in xclim.

Finally, I also had to remove the annotations from xclim.sdba.Grouper.group. I think returning a GroupBy object is ok, but it isn't itself public API, only its methods are... It could be moved around in the module without warning.

@Zeitsperre Zeitsperre merged commit 0667dfd into main Mar 25, 2025
23 checks passed
@Zeitsperre Zeitsperre deleted the revert-star-annotation branch March 25, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests bug Something isn't working CI Automation and Contiunous Integration docs Improvements to documenation indicators Climate indices and indicators sdba Issues concerning the sdba submodule.
Projects
None yet
5 participants