Support keyword API for `Dataset.drop` #3128

gwgundersen · 2019-07-14T17:36:03Z

Closes Keyword argument support for drop() #2910.
In tests/test_dataset, added test_drop_labels_by_keyword
Added a utility function is_list_like.
Documented in whats-new.rst and confirmed API docs built correctly.

codecov · 2019-07-14T17:39:13Z

Codecov Report

Merging #3128 into master will increase coverage by 0.31%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #3128      +/-   ##
==========================================
+ Coverage   95.66%   95.98%   +0.31%     
==========================================
  Files          64       63       -1     
  Lines       13117    12788     -329     
==========================================
- Hits        12548    12274     -274     
+ Misses        569      514      -55

shoyer · 2019-07-14T19:41:38Z

Awesome, thanks for working on this @gwgundersen !

In the long term, we might want to deprecate the current function signature, but I'm happy to leave that for later.

xarray/core/dataset.py

max-sixty · 2019-08-15T22:23:07Z

@gwgundersen I see you trying to merge re the recent black changes. Here are the instructions for making this (relatively) seamless: https://github.com/pydata/xarray/blob/d3f1ced1aa84cb0a0f3e47e7ee67492e203fcce5/doc/contributing.rst#code-formatting

gwgundersen · 2019-08-15T22:29:36Z

@max-sixty, thanks! It's useful to be able to do in advance.

After merging from master, I get this error:

flake8
./xarray/core/dataset.py:3466:5: F811 redefinition of unused 'drop' from line 3460

I assume this is due to #3177 cc @crusaderky. Is there a way to avoid this?

max-sixty · 2019-08-15T23:08:42Z

@gwgundersen Could you force-push your latest code?

If your current latest code is pushed, you may have to git reset --hard and start again - unfortunately it looks like your PR is proposing +24,439 −17,647 changes

pep8speaks · 2019-08-16T06:46:32Z

Hello @gwgundersen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-18 07:25:58 UTC

gwgundersen · 2019-08-16T07:00:37Z

Sorry about that. I'm not sure what happened. I've reverted branch and cherry picked my changes. The issue with drop being a duplicated name still stands.

xarray/core/dataset.py

max-sixty

Added a noqa suggestion so flake doesn't complain

max-sixty · 2019-08-16T15:50:06Z

Thanks @gwgundersen - good idea re the cherry pick for more discrete changes.

Still getting a test failure though, check it out and lmk if you need any help

…inates.

gwgundersen · 2019-08-17T10:22:58Z

Now the docs don't build. When I try to build locally, I get a vague error:

  File "/Users/gwg/miniconda3/envs/xarray-docs/lib/python3.7/site-packages/IPython/sphinxext/ipython_directive.py", line 583, in process_input
    raise RuntimeError('Non Expected warning in `{}` line {}'.format(filename, lineno))
RuntimeError: Non Expected warning in `/Users/gwg/projects/xarray/doc/indexing.rst` line 241

Do you see anything wrong with this?

The :py:meth:`~xarray.Dataset.drop` method returns a new object with the listed
index labels along a dimension dropped:

.. ipython:: python

    ds.drop(['IN', 'IL'], dim='space')
<-- line 241
``drop`` is both a ``Dataset`` and ``DataArray`` method.

Use :py:meth:`~xarray.Dataset.drop_dims` to drop a full dimension from a Dataset.
Any variables with these dimensions are also dropped:

dcherian · 2019-08-17T14:21:27Z

It looks like you've added a warming for this case. So you'll need to specify :okwarning: .

max-sixty · 2019-08-17T15:42:10Z

I see the docs timing out after 60 min. I also see that message, but is that causing the failure?

I restarted the docs build to see whether that might help

… dimensions.

gwgundersen · 2019-08-17T22:34:21Z

Sorry for the back-and-forth. CI tests and checks are all passing, and this is ready for another review.

max-sixty

Super! Will merge tomorrow unless anyone has comments?

xarray/core/dataset.py

max-sixty · 2019-08-17T23:43:20Z

@gwgundersen could we add a whatsnew too?

Co-Authored-By: Maximilian Roos <[email protected]>

…eyword-support

…into drop-keyword-support

gwgundersen · 2019-08-18T07:42:58Z

@max-sixty, done.

max-sixty · 2019-08-18T17:42:54Z

Great @gwgundersen !

shoyer

@gwgundersen thanks for this PR!

I have a few minor points that would be good to address before this goes out in an official release.

shoyer · 2019-08-18T20:24:33Z

xarray/core/dataset.py

        """
        if errors not in ["raise", "ignore"]:
            raise ValueError('errors must be either "raise" or "ignore"')

-        if dim is None:
+        labels_are_coords = isinstance(labels, DataArrayCoordinates)
+        if labels_kwargs or (utils.is_dict_like(labels) and not labels_are_coords):


I wonder if it would be more predictable to special case the dict type rather than not labels_are_coords, which is just one of many internal xarray mapping types, e.g.,

Suggested change

if labels_kwargs or (utils.is_dict_like(labels) and not labels_are_coords):

if labels_kwargs or isinstance(labels, dict):

More generally, I get that writing array.drop(array.coords) is convenient, but in the long term it seems like a bad idea to treat different mapping types differently. This suggests that such uses should be deprecated. Either array.drop_vars(array.coords) or array.drop(list(array.coords)) would be less ambiguous.

Yeah, I thought needing the check for labels_are_coords was odd, but I didn't expect passing in coordinates to take a separate code path. Special-casing the dict type makes sense. And the API that is being deprecated is treating DataArrayCoordinates as non-dict-like, right?

And the API that is being deprecated is treating DataArrayCoordinates as non-dict-like, right?

That's right, we would rather always use this new rule for dict-like inputs.

shoyer · 2019-08-18T20:25:19Z

doc/indexing.rst

@@ -236,6 +236,7 @@ The :py:meth:`~xarray.Dataset.drop` method returns a new object with the listed
 index labels along a dimension dropped:

 .. ipython:: python
+    :okwarning:

    ds.drop(['IN', 'IL'], dim='space')


Since this is deprecated, we should fix the docs to use ds.drop(space=['IN', 'IL'] as an example of the preferred syntax going forward.

max-sixty · 2019-08-18T20:43:55Z

@shoyer thanks for the follow-up comments. I probably should have spotted those myself.
But given there'll be points I miss, lmk if I should be waiting longer / raising specific points before hitting the big green button, or these post-merge reviews are OK. I can ensure they're complete before a release.

shoyer · 2019-08-18T20:56:29Z

Post merge reviews are totally fine. It is not a big deal to revert a merge if needed.

gwgundersen · 2019-08-18T21:43:57Z

Happy to make these changes, but what's the protocol here? Seems like reverting the merge and letting me amend is cleaner than me creating a new PR.

This reverts commit d578bfe.

shoyer · 2019-08-18T21:53:08Z

My (limited) understanding of how github handles reverting is that you'll still need to make a new pull request to apply your edits either way. There is no more going to back to amend after PR is merges. The only difference is what the state of "master" looks like when you make your new pull request (whether it includes your partial changes or not).

So I don't think it particularly matters if you revert first (as long as the forward fix happens relatively soon), but if you'd prefer to do it that way that's totally fine -- let me know and I will do it.

max-sixty · 2019-08-19T02:24:00Z

I think the easiest way forward is to do a follow-up PR, no need to revert.

Reverting would be helpful if we wanted to undo these changes - in this case we have some v marginal additional changes.

Support for keyword argument-based dropping.

4b78b68

gwgundersen mentioned this pull request Jul 14, 2019

Keyword argument support for drop() #2910

Closed

shoyer reviewed Jul 14, 2019

View reviewed changes

xarray/core/dataset.py Outdated Show resolved Hide resolved

xarray/core/dataset.py Outdated Show resolved Hide resolved

xarray/core/dataset.py Show resolved Hide resolved

xarray/core/dataset.py Outdated Show resolved Hide resolved

shoyer mentioned this pull request Aug 4, 2019

More annotations #3177

Merged

Reverting.

1de442e

gwgundersen force-pushed the drop-keyword-support branch from 97271bd to 1de442e Compare August 16, 2019 06:46

Cherry picked changes.

553e548

max-sixty reviewed Aug 16, 2019

View reviewed changes

xarray/core/dataset.py Outdated Show resolved Hide resolved

max-sixty reviewed Aug 16, 2019

View reviewed changes

Moved noqa to correct location; added check for when labels are coord…

5aa8dc1

…inates.

gwgundersen added 3 commits August 17, 2019 21:54

Added okwarning to drop docs.

49f7d83

Made unit tests more explicit. Added test for dropping along multiple…

e3f051c

… dimensions.

Used black.

c150197

max-sixty approved these changes Aug 17, 2019

View reviewed changes

xarray/core/dataset.py Outdated Show resolved Hide resolved

gwgundersen and others added 3 commits August 18, 2019 08:05

Fixed typo.

2222f0b

Co-Authored-By: Maximilian Roos <[email protected]>

Merge branch 'master' of https://github.com/pydata/xarray into drop-k…

a225b80

…eyword-support

Docs for amended drop API.

4fc46ba

Merge branch 'drop-keyword-support' of github.com:gwgundersen/xarray …

6074550

…into drop-keyword-support

max-sixty merged commit d578bfe into pydata:master Aug 18, 2019

gwgundersen deleted the drop-keyword-support branch August 18, 2019 17:51

shoyer reviewed Aug 18, 2019

View reviewed changes

shoyer added a commit that referenced this pull request Aug 18, 2019

Revert "Support keyword API for Dataset.drop (#3128)"

be15ee4

This reverts commit d578bfe.

This was referenced Aug 19, 2019

Dict-like argument support for assign_coords() #3231

Closed

Drop keyword support: small fixes #3233

Merged

	if labels_kwargs or (utils.is_dict_like(labels) and not labels_are_coords):
	if labels_kwargs or isinstance(labels, dict):

Uh oh!

Support keyword API for Dataset.drop #3128

Support keyword API for Dataset.drop #3128

Uh oh!

Conversation

gwgundersen commented Jul 14, 2019

Uh oh!

codecov bot commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shoyer commented Jul 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

max-sixty commented Aug 15, 2019

Uh oh!

gwgundersen commented Aug 15, 2019

Uh oh!

max-sixty commented Aug 15, 2019

Uh oh!

pep8speaks commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2019-08-18 07:25:58 UTC

Uh oh!

gwgundersen commented Aug 16, 2019

Uh oh!

Uh oh!

max-sixty left a comment

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Aug 16, 2019

Uh oh!

gwgundersen commented Aug 17, 2019

Uh oh!

dcherian commented Aug 17, 2019

Uh oh!

max-sixty commented Aug 17, 2019

Uh oh!

gwgundersen commented Aug 17, 2019

Uh oh!

max-sixty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

max-sixty commented Aug 17, 2019

Uh oh!

gwgundersen commented Aug 18, 2019

Uh oh!

max-sixty commented Aug 18, 2019

Uh oh!

shoyer left a comment

Choose a reason for hiding this comment

Uh oh!

shoyer Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

gwgundersen Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

shoyer Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

shoyer Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

max-sixty commented Aug 18, 2019

Uh oh!

shoyer commented Aug 18, 2019

Uh oh!

gwgundersen commented Aug 18, 2019

Uh oh!

shoyer commented Aug 18, 2019

Uh oh!

max-sixty commented Aug 19, 2019

Uh oh!

Uh oh!

Support keyword API for `Dataset.drop` #3128

Support keyword API for `Dataset.drop` #3128

codecov bot commented Jul 14, 2019 •

edited

Loading

pep8speaks commented Aug 16, 2019 •

edited

Loading