Skip to content

Conversation

mathause
Copy link
Collaborator

@mathause mathause commented Mar 23, 2022

  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

This makes many arguments keyword-only, except for dim to avoid da.weighted(...).mean("lat", "lon") (i.e. da.weighted(...).mean(dim="lat", skipna="lon")) which silently does the wrong thing. I am sure I forgot some and for some I was unsure so I left them as is.

Question: do we want an deprecation cycle? Currently it just errors for da.weighted(...).mean("dim", True). Might be nice to do it, however, @dcherian if I am not mistaken you did this without a deprecation in #5950, e.g. for da.mean etc.?

import numpy as np
import xarray as xr
air = xr.tutorial.open_dataset("air_temperature")
wgt = np.cos(np.deg2rad(air.lat))
air.weighted(wgt).mean("lat", "lon")

@max-sixty
Copy link
Collaborator

This is great, thanks @mathause .

From what I've seen in the wild, I do think we're going to get some breaks on some of these without a deprecation — e.g. I've rarely seen an n arg used for diff . What are your thoughts on the link that @gcaria posted in #5531? That might do the deprecation for us.

@mathause
Copy link
Collaborator Author

Sounds good - I'll add a deprecation.

We might also want to do this for https://github.com/pydata/xarray/blob/main/xarray/core/_reductions.py

1 similar comment
@mathause
Copy link
Collaborator Author

Sounds good - I'll add a deprecation.

We might also want to do this for https://github.com/pydata/xarray/blob/main/xarray/core/_reductions.py

@dcherian
Copy link
Contributor

We might also want to do this for https://github.com/pydata/xarray/blob/main/xarray/core/_reductions.py

👍 I lost track of it given that PR was open for so long. IIRC one issue is that the order of args was different forr Dataset.reduce vs DatasetGroupBy.reduce (cc @Illviljan)

@mathause
Copy link
Collaborator Author

From #5950 (comment)

Keyword-only and some had the 3rd position arg as keep_attrs and in other cases it was axis.

@Illviljan
Copy link
Contributor

Illviljan commented Mar 23, 2022

Yes, mypy found some issues with the ordering so moving to kwargs-only was my preferred solution.
It indeed was a breaking change but I didn't think keep_attrs, axis, etc were that popular arguments either so I was not expecting too many noticing the change, and maybe just a note in the whats new would've been sufficient.

@max-sixty
Copy link
Collaborator

I would vote to resurrect this! Deprecations are nice but I would vote to merge this without them given that it seems that we won't merge it with them... (We can do it on a minor release and announce it)

What do folks think?

Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

I'm ok with merging this.

Maybe relax it a bit for the diff method, I remember using n as a positional argument in the past (coming from Matlab).

Should get an entry in breaking changes in the what's new though.

@mathause
Copy link
Collaborator Author

mathause commented Oct 2, 2023

Happy to revive this PR. We do have a @_deprecate_positional_args helper - I added this as well (IIRC it was added after opening this PR). I am sure we could make even more arguments positional but I should be doing my actual job ;-)

@mathause
Copy link
Collaborator Author

mathause commented Oct 2, 2023

I think the failure is a fluke. Open/ close

@mathause mathause closed this Oct 2, 2023
@mathause mathause reopened this Oct 2, 2023
@mathause mathause marked this pull request as ready for review October 2, 2023 14:14
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @mathause

Let's add a whats-new note and merge.

Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks!

@headtr1ck headtr1ck added the plan to merge Final call for comments label Oct 4, 2023
@dcherian dcherian enabled auto-merge (squash) October 5, 2023 18:39
@dcherian dcherian merged commit 938579d into pydata:main Oct 5, 2023
dcherian added a commit to dcherian/xarray that referenced this pull request Oct 14, 2023
* upstream/main: (46 commits)
  xfail flaky test (pydata#8299)
  Most of mypy 1.6.0 passing (pydata#8296)
  Rename `reset_encoding` to `drop_encoding` (pydata#8287)
  Enable `.rolling_exp` to work on dask arrays (pydata#8284)
  Fix `GroupBy` import (pydata#8286)
  Ask bug reporters to confirm they're using a recent version of xarray (pydata#8283)
  Add pyright type checker (pydata#8279)
  Improved typing of align & broadcast (pydata#8234)
  Update ci-additional.yaml (pydata#8280)
  Fix time encoding regression (pydata#8272)
  Allow a function in `.sortby` method (pydata#8273)
  make more args kw only (except 'dim') (pydata#6403)
  Use duck array ops in more places (pydata#8267)
  Don't raise rename warning if it is a no operation (pydata#8266)
  Mandate kwargs on `to_zarr` (pydata#8257)
  copy  the `dtypes` module to the `namedarray` package. (pydata#8250)
  Add xarray-regrid to ecosystem.rst (pydata#8270)
  Use strict type hinting for namedarray (pydata#8241)
  Update type annotation for center argument of dataaray_plot methods (pydata#8261)
  [pre-commit.ci] pre-commit autoupdate (pydata#8262)
  ...
@mathause mathause deleted the less_positional_args branch June 6, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments topic-groupby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants