drop_vars; deprecate drop for variables by max-sixty · Pull Request #3475 · pydata/xarray

max-sixty · 2019-10-31T18:46:48Z

Introduces drop_vars, and deprecates using drop for variables. drop is widely used for the deprecated case, so this is a fairly wide blast radius.

It's more churn than is ideal, but I do think it's a much better API.

This is ready for review, though I'm sure I'm missed references in the docs etc (took my peak regex skills to find/replace only the deprecated usages!)

Originally discussed here

Tests added
Passes black . && mypy . && flake8
Fully documented, including whats-new.rst for all changes and api.rst for new API

# Conflicts: # xarray/core/dataarray.py

shoyer · 2019-11-05T01:55:38Z

What do you think about this alternative?

Add drop_sel for dropping labels
Add drop_vars for variables.
Keep around drop as an overloaded alias for both?

(I'm not really sure which is best, to be honest. But keeping down churn does seem like a good idea, if possible.)

max-sixty · 2019-11-05T04:15:05Z

Yes, that sounds reasonable—I think we move documentation and internal usages to drop_sel and drop_vars, and leave drop as a backward compatible option?

Maybe at last a case for FutureDeprecationWarning :)

max-sixty · 2019-11-06T00:20:12Z

Updated! I used PendingDeprecationWarning, which is default disabled.

max-sixty · 2019-11-06T00:29:42Z

@crusaderky let me know if you know off-hand why the MinimumVersionsPolicy is breaking, otherwise I can look into it

dcherian · 2019-11-06T15:14:46Z

Add drop_sel for dropping labels

Would drop_labels be better?

max-sixty · 2019-11-06T21:03:07Z

Would drop_labels be better?

I think either is reasonable. drop_sel has the advantage that it's easily comparable to sel, and has the same arguments, so I'd marginally vote for that. Am easily swayed by popular opinion!

shoyer · 2019-11-06T21:04:12Z

I wanted a name that is reminiscent of "sel" in some way.

…

On Wed, Nov 6, 2019 at 7:14 AM Deepak Cherian ***@***.***> wrote: Add drop_sel for dropping labels Would drop_labels be better? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3475?email_source=notifications&email_token=AAJJFVVMPYUZS37B4BW2C43QSLNOPA5CNFSM4JHQAN7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDG4BNQ#issuecomment-550355126>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJJFVQKLRALMGU5ALWKWVLQSLNOPANCNFSM4JHQAN7A> .

dcherian · 2019-11-06T21:49:43Z

👍

max-sixty · 2019-11-07T17:36:42Z

This is ready to go!

As a reminder, we have PendingDeprecationWarning for drop, which is ignored in user code by default. It does appear in tests, so let me know if that's too strong given the above discussion on backward-compat.

dcherian

LGTM. just some minor doc comments

doc/data-structures.rst

doc/indexing.rst

xarray/core/dataarray.py

dcherian · 2019-11-07T17:49:18Z

xarray/core/dataset.py

+        dropped : Dataset
+
+        """
+        if isinstance(names, str) or not isinstance(names, Iterable):


not sure if this should be isinstance(names, str) or is_scalar(names)?

Good spot; I've changed this. But had to keep or not isinstance(names, Iterable): for mypy

@crusaderky as the resident expert, lmk if you have any thoughts on this.

max-sixty · 2019-11-07T20:13:42Z

Going to merge given @dcherian 's approval, any other follow-ups I can address in another PR

…tests * upstream/master: drop_vars; deprecate drop for variables (pydata#3475) uamiv test using only raw uamiv variables (pydata#3485) Optimize dask array equality checks. (pydata#3453)

shoyer · 2019-11-07T23:19:23Z

doc/whats-new.rst

+- :py:meth:`Dataset.drop_sel` & :py:meth:`DataArray.drop_sel` have been added for dropping labels.
+  :py:meth:`Dataset.drop_vars` & :py:meth:`DataArray.drop_vars` have been added for 
+  dropping variables (including coordinates). The existing ``drop`` methods remain as a backward compatible 
+  option for dropping either lables or variables, but using the more specific methods is encouraged.


lables > labels

shoyer · 2019-11-07T23:20:40Z

Thank you @max-sixty, and @dcherian for reviewing!

* upstream/master: (27 commits) drop_vars; deprecate drop for variables (pydata#3475) uamiv test using only raw uamiv variables (pydata#3485) Optimize dask array equality checks. (pydata#3453) Propagate indexes in DataArray binary operations. (pydata#3481) python 3.8 tests (pydata#3477) __dask_tokenize__ (pydata#3446) Type check sentinel values (pydata#3472) Fix typo in docstring (pydata#3474) fix test suite warnings re `drop` (pydata#3460) Fix integrate docs (pydata#3469) Fix leap year condition in monthly means example (pydata#3464) Hypothesis tests for roundtrip to & from pandas (pydata#3285) unpin cftime (pydata#3463) Cleanup whatsnew (pydata#3462) enable xr.ALL_DIMS in xr.dot (pydata#3424) Merge stable into master (pydata#3457) upgrade black verison to 19.10b0 (pydata#3456) Remove outdated code related to compatibility with netcdftime (pydata#3450) Remove deprecated behavior from dataset.drop docstring (pydata#3451) jupyterlab dark theme (pydata#3443) ...

* upstream/master: add missing pint integration tests (pydata#3508) DOC: update bottleneck repo url (pydata#3507) add drop_sel, drop_vars, map to api.rst (pydata#3506) remove syntax warning (pydata#3505) Dataset.map, GroupBy.map, Resample.map (pydata#3459) tests for datasets with units (pydata#3447) fix pandas-dev tests (pydata#3491) unpin pseudonetcdf (pydata#3496) whatsnew corrections (pydata#3494) drop_vars; deprecate drop for variables (pydata#3475) uamiv test using only raw uamiv variables (pydata#3485) Optimize dask array equality checks. (pydata#3453)

* upstream/master: format indexing.rst code with black (pydata#3511) add missing pint integration tests (pydata#3508) DOC: update bottleneck repo url (pydata#3507) add drop_sel, drop_vars, map to api.rst (pydata#3506) remove syntax warning (pydata#3505) Dataset.map, GroupBy.map, Resample.map (pydata#3459) tests for datasets with units (pydata#3447) fix pandas-dev tests (pydata#3491) unpin pseudonetcdf (pydata#3496) whatsnew corrections (pydata#3494) drop_vars; deprecate drop for variables (pydata#3475) uamiv test using only raw uamiv variables (pydata#3485) Optimize dask array equality checks. (pydata#3453) Propagate indexes in DataArray binary operations. (pydata#3481) python 3.8 tests (pydata#3477)

commit d430ae0 Author: dcherian <deepak@cherian.net> Date: Wed Nov 13 08:27:04 2019 -0700 proper fix. commit 7fd69be Author: dcherian <deepak@cherian.net> Date: Wed Nov 13 08:03:26 2019 -0700 fix whats-new merge. commit 4489394 Merge: 279ff1d b74f80c Author: dcherian <deepak@cherian.net> Date: Wed Nov 13 08:03:06 2019 -0700 Merge remote-tracking branch 'upstream/master' into fix/plot-broadcast * upstream/master: format indexing.rst code with black (pydata#3511) add missing pint integration tests (pydata#3508) DOC: update bottleneck repo url (pydata#3507) add drop_sel, drop_vars, map to api.rst (pydata#3506) remove syntax warning (pydata#3505) Dataset.map, GroupBy.map, Resample.map (pydata#3459) tests for datasets with units (pydata#3447) fix pandas-dev tests (pydata#3491) unpin pseudonetcdf (pydata#3496) whatsnew corrections (pydata#3494) drop_vars; deprecate drop for variables (pydata#3475) uamiv test using only raw uamiv variables (pydata#3485) Optimize dask array equality checks. (pydata#3453) Propagate indexes in DataArray binary operations. (pydata#3481) python 3.8 tests (pydata#3477) commit 279ff1d Author: dcherian <deepak@cherian.net> Date: Wed Nov 13 08:02:44 2019 -0700 Undo the transpose change and add test to make sure transposition is right. commit c9cc698 Author: dcherian <deepak@cherian.net> Date: Wed Nov 13 08:01:39 2019 -0700 Test to make sure transpose is right commit 9b35ecf Author: dcherian <deepak@cherian.net> Date: Sat Nov 2 15:49:08 2019 -0600 Additional test. commit 7aed950 Author: dcherian <deepak@cherian.net> Date: Sat Nov 2 15:20:07 2019 -0600 make plotting work with transposed nondim coords.

max-sixty added 6 commits October 31, 2019 14:19

Deprecate drop for vars, in favor of drop_vars

399f9d6

Merge branch 'master' into drop

222a31b

# Conflicts: # xarray/core/dataarray.py

docs tweaks

5a2604c

handle scalars as vars

5126bd1

allow warning in old whatsnew

3651ed8

Merge branch 'master' into drop

64007ac

max-sixty added 7 commits November 5, 2019 18:26

Merge branch 'master' into drop

1284cd2

add drop_sel, adjust deprecations based on comments

e72693b

whatsnew

a8467c1

docs

4b8db19

old-whatsnew

13d6cab

docstring

a8dab28

pendingdeprecationwarning

7002cb3

whatsnew

5570643

whatsnew

3997292

dcherian approved these changes Nov 7, 2019

View reviewed changes

max-sixty added 2 commits November 7, 2019 13:17

move units tests to drop_sel

0de1411

is_scalar (but retain isinstance for mypy)

db4b2a3

max-sixty merged commit 0e8debf into pydata:master Nov 7, 2019

max-sixty deleted the drop branch November 7, 2019 20:13

shoyer reviewed Nov 7, 2019

View reviewed changes

max-sixty mentioned this pull request Nov 7, 2019

Whatsnew corrections #3494

Merged

keewis mentioned this pull request Nov 20, 2019

Dataset.drop() no longer works with a set as an argument #3552

Closed

ttung mentioned this pull request Dec 2, 2019

[easy] update requirements spacetx/starfish#1683

Merged

brews mentioned this pull request Feb 10, 2022

drop spatial bounds coordinates if present in cmip6 cleaning ClimateImpactLab/dodola#177

Closed

4 tasks

Uh oh!

Conversation

max-sixty commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shoyer commented Nov 5, 2019

Uh oh!

max-sixty commented Nov 5, 2019

Uh oh!

max-sixty commented Nov 6, 2019

Uh oh!

max-sixty commented Nov 6, 2019

Uh oh!

dcherian commented Nov 6, 2019

Uh oh!

max-sixty commented Nov 6, 2019

Uh oh!

shoyer commented Nov 6, 2019 via email

Uh oh!

dcherian commented Nov 6, 2019

Uh oh!

max-sixty commented Nov 7, 2019

Uh oh!

dcherian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcherian Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

max-sixty Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Nov 7, 2019

Uh oh!

shoyer Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

shoyer commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

max-sixty commented Oct 31, 2019 •

edited

Loading