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

Deprecate inplace methods #1756

Closed
shoyer opened this issue Dec 2, 2017 · 6 comments · Fixed by #2524
Closed

Deprecate inplace methods #1756

shoyer opened this issue Dec 2, 2017 · 6 comments · Fixed by #2524
Milestone

Comments

@shoyer
Copy link
Member

shoyer commented Dec 2, 2017

The following methods have an inplace argument:
DataArray.reset_coords
DataArray.set_index
DataArray.reset_index
DataArray.reorder_levels
Dataset.set_coords
Dataset.reset_coords
Dataset.rename
Dataset.swap_dims
Dataset.set_index
Dataset.reset_index
Dataset.reorder_levels
Dataset.update
Dataset.merge

As proposed in #1755 (comment), let's deprecate all of these at the next major release (v0.11). They add unnecessary complexity to methods and promote confusing about xarray's data model.

Practically, we would change all of the default values to inplace=None and issue either a DeprecationWarning or FutureWarning (see PEP 565 for more details on that choice).

@st-bender
Copy link
Contributor

Hi,
Sorry for the late comment about a closed bug. But I find changing the API a bit irritating to say the least, and this is a serious change. Although apparently not many people use it, some actually may (myself included). And so far there has been only one bug report, so what problem are you trying to fix?
I can fix my own code but there may be others out there that cannot keep pace with the development and including their packages may then break software. For my taste the deprecation warning is a bit short if you are going to remove such a feature already in the next version. A few more cycles would be appreciated.

At the very least put a big warning sign to the documentation that xarray is still beta and the API is still subject to change.

@jhamman
Copy link
Member

jhamman commented Jan 25, 2019

@st-bender - sorry to hear this change has caught you by surprise. We're a ways off from 0.12 so you should have time to work in any necessary changes before the feature if fully removed. If you have questions about how to adjust our use these methods, let us know.

@shoyer
Copy link
Member Author

shoyer commented Jan 25, 2019

We will probably actually wait longer than until 0.12 for removing this argument, depending on the timing for that release.

Unfortunately our API is indeed not entirely stable yet -- that's why the version number is still 0.X, not 1.X.

@st-bender
Copy link
Contributor

Hi,
Thanks for the replies, I was indeed caught by surprise and given that the version number is 0.11.x, I had the impression that 0.12.x would be the next major minor release (and coming soon).

@shoyer In that case I would take it back and vote for a change as soon as possible to stabilize the API. Although xarray is still considered beta, I guess some people already use it productively.

@jhamman Thanks for the offer, I think the changes were simple enough. I merely wanted to point out that some more people use(d) that feature.

@nathanlyons
Copy link

Is there a recommendation for merging a dataset without inplacewhen the dataset is a subclass of xr.Dataset, other than through composition? Perhaps lower level functions to merge?

@shoyer
Copy link
Member Author

shoyer commented Mar 25, 2019 via email

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

Successfully merging a pull request may close this issue.

5 participants