issue_comments

Merging but I or @ahuang11 can do any follow-ups.

Thank you v much @ahuang11 — appreciate this was a lot of work and happy to have you contribute such a good feature.

@shoyer could I confirm that limiting to a single dimension satisfies this? You say "multiple variables" here. I mentioned a single dimension on the call but maybe wasn't clear.

@shoyer can you kindly merge this if you're happy with it?

Hello @ahuang11! 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! :beers:

Comment last updated at 2021-05-13 17:47:30 UTC

Dont understand this xarray/core/dataarray.py:4605: error: Argument 1 to "isel" of "DataArray" has incompatible type "Dict[str, Any]"; expected "Optional[Mapping[Hashable, Any]]" [arg-type] Found 1 error in 1 file (checked 140 source files)

self.isel(indexes)

@shoyer could I confirm that limiting to a single dimension satisfies this? You say "multiple variables" here. I mentioned a single dimension on the call but maybe wasn't clear.

If you drop duplicates over multiple variables at once, what dimensions should the result have? In particular -- should it have the original dimensions, or should all dimensions involved be combined into one?

The latter might seem a little crazy now, but would make more sense once we allow dropping over multi-dimensional variables.

If we really want to avoid any possible controversy here, it might be best to stick to only supporting one variable for now, too.

Sure.

What's the reasoning for a single dimension?

On Wed, May 12, 2021, 11:34 AM Maximilian Roos @.***> wrote:

@ahuang11 https://github.com/ahuang11 we discussed this on the core team call. People are excited to merge this, and appreciate you bearing with the changes.

I suggested that we narrow this even further to one dimension and merge, so we can benefit from this now and consider additions from there. Would that be OK with you?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/5239#issuecomment-839925301, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADU7FFSS2IQ5QP7BYG4OCWLTNKUXXANCNFSM4353OVNA .

@ahuang11 we discussed this on the core team call. People are excited to merge this, and appreciate you bearing with the changes.

I suggested that we narrow this even further to one dimension and merge, so we can benefit from this now and consider additions from there. Would that be OK with you?

the docs build can probably be fixed by adding newlines before the section headers ("Parameters" / "Returns")

If you drop duplicates over multiple variables at once, what dimensions should the result have? In particular -- should it have the original dimensions, or should all dimensions involved be combined into one?

The latter might seem a little crazy now, but would make more sense once we allow dropping over multi-dimensional variables.

If we really want to avoid any possible controversy here, it might be best to stick to only supporting one variable for now, too.

For me this case supports consistent behavior over any number of dims! Which would mean rejecting the "stack only if you supply exactly ndims" proposal.

Completely fine with restricting to one dim for the moment and seeing how that goes (it's always possible to pass .drop_duplicates_coords(dim1).drop_duplicates_coords(dim2).

Were we going with drop_duplicates or drop_duplicates_coords? No strong view from me at all.

The docs are failing with:

/home/docs/checkouts/readthedocs.org/user_builds/xray/checkouts/5239/xarray/core/dataarray.py:docstring of xarray.core.dataarray.DataArray.drop_duplicates:3: WARNING: Unexpected indentation. /home/docs/checkouts/readthedocs.org/user_builds/xray/checkouts/5239/xarray/core/dataarray.py:docstring of xarray.core.dataarray.DataArray.drop_duplicates:4: WARNING: Block quote ends without a blank line; unexpected unindent. looking for now-outdated files... none found