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.

Sign up for GitHub

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

Jump to bottom

change ALL_DIMS to equal ellipsis #3418

Merged
merged 9 commits into from
Oct 25, 2019
Merged

change ALL_DIMS to equal ellipsis #3418

merged 9 commits into from
Oct 25, 2019

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Oct 19, 2019 •
edited
Loading

This is a more drastic version of the suggested change, making xr.ALL_DIMS equal the ellipsis, ....

The only downside I can see is that printing xr.ALL_DIMS will print .... The upside is that we get compat 'for free'

@crusaderky
Copy link
Contributor

Tests broken by #3409 - please stand by...

@max-sixty
Copy link
Collaborator Author

Should we advertise ... as the recommended way to supply 'all dims', rather than xr.ALL_DIMS? (if we're not sure, we could leave as currently proposed and change later)

@crusaderky
Copy link
Contributor

@max-sixty I'm in favour of ... as the preferred syntax

@max-sixty
Copy link
Collaborator Author

Ready for review / merge

@crusaderky crusaderky mentioned this pull request Oct 22, 2019
Closed
12 tasks
@max-sixty
Copy link
Collaborator Author

Hmmm, still getting cftime issues with the build: ERROR: Command errored out with exit status 128: git fetch -q https://github.com/Unidata/cftime.git refs/pull/127/merge Check the logs for full command output.
https://dev.azure.com/xarray/xarray/_build/results?buildId=1208&view=logs

@dcherian
Copy link
Contributor

cftime has merged the PR with the fix so we should just switch back to their master branch

@shoyer
Copy link
Member

shoyer commented Oct 24, 2019

build failure should be fixed by #3439

@max-sixty
Copy link
Collaborator Author

Any final comments before merge?

dcherian
dcherian approved these changes Oct 24, 2019
View reviewed changes
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.

minor comments. LGTM.

xarray/core/groupby.py Outdated Show resolved Hide resolved
doc/groupby.rst Show resolved Hide resolved
xarray/core/groupby.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor

Thanks @max-sixty !

@dcherian dcherian merged commit 79b3cdd into pydata:master Oct 25, 2019
dcherian added a commit to dcherian/xarray that referenced this pull request Oct 25, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into fix/groupby-reduc…
228c5c7 
…e-multiple-dims

* upstream/master:
  change ALL_DIMS to equal ellipsis (pydata#3418)
  Escaping dtypes (pydata#3444)
  Html repr (pydata#3425)
@max-sixty max-sixty deleted the ellipsis branch October 25, 2019 17:16
dcherian added a commit to dcherian/xarray that referenced this pull request Oct 28, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into fix/groupby-nan
c396381 
* upstream/master:
  Another groupby.reduce bugfix. (pydata#3403)
  add icomoon license (pydata#3448)
  change ALL_DIMS to equal ellipsis (pydata#3418)
  Escaping dtypes (pydata#3444)
  Html repr (pydata#3425)
dcherian added a commit to dcherian/xarray that referenced this pull request Oct 29, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into dask-tokenize
bc39877 
* upstream/master:
  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)
  Drop groups associated with nans in group variable (pydata#3406)
  Allow ellipsis (...) in transpose (pydata#3421)
  Another groupby.reduce bugfix. (pydata#3403)
  add icomoon license (pydata#3448)
  change ALL_DIMS to equal ellipsis (pydata#3418)
  Escaping dtypes (pydata#3444)
  Html repr (pydata#3425)
dcherian added a commit to dcherian/xarray that referenced this pull request Oct 30, 2019
@dcherian
Squashed commit of the following:
a47ff7b 
commit bc39877
Merge: 507b1f6 278d2e6
Author: dcherian <[email protected]>
Date:   Tue Oct 29 09:36:30 2019 -0600

    Merge remote-tracking branch 'upstream/master' into dask-tokenize

    * upstream/master:
      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)
      Drop groups associated with nans in group variable (pydata#3406)
      Allow ellipsis (...) in transpose (pydata#3421)
      Another groupby.reduce bugfix. (pydata#3403)
      add icomoon license (pydata#3448)
      change ALL_DIMS to equal ellipsis (pydata#3418)
      Escaping dtypes (pydata#3444)
      Html repr (pydata#3425)

commit 507b1f6
Author: dcherian <[email protected]>
Date:   Tue Oct 29 09:34:47 2019 -0600

    Fix window test

commit 4ab6a66
Author: dcherian <[email protected]>
Date:   Thu Oct 24 14:30:57 2019 -0600

    Implement __dask_tokenize__
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@max-sixty @crusaderky @dcherian @shoyer