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

enable xr.ALL_DIMS in xr.dot #3424

Merged
merged 7 commits into from
Oct 29, 2019
Merged

enable xr.ALL_DIMS in xr.dot #3424

merged 7 commits into from
Oct 29, 2019

Conversation

mathause
Copy link
Collaborator

@mathause mathause commented Oct 21, 2019 •
edited
Loading

  • Closes add ALL_DIMS in xr.dot #3423
  • Tests added
  • Passes black . && mypy . && flake8
  • Fully documented, including whats-new.rst for all changes and api.rst for new API

@pep8speaks
Copy link

pep8speaks commented Oct 21, 2019 •
edited
Loading

Hello @mathause! 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-10-29 17:26:42 UTC

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

Would you like me to replace xr.ALL_DIMS with ...? (Although here I kind of like the explicit xr.ALL_DIMS).

@shoyer
Copy link
Member

shoyer commented Oct 23, 2019

It's fine to stick with ALL_DIMS for now. We'll remap ALL_DIMS = ... later, but every existing usage of ALL_DIMS will still work.

max-sixty
max-sixty reviewed Oct 29, 2019
View reviewed changes
xarray/core/computation.py Outdated
@@ -1055,7 +1056,7 @@ def dot(*arrays, dims=None, **kwargs):
----------
arrays: DataArray (or Variable) objects
Arrays to compute.
dims: str or tuple of strings, optional
dims: xarray.ALL_DIMS, str or tuple of strings, optional
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dims: xarray.ALL_DIMS, str or tuple of strings, optional
dims: '...', str or tuple of strings, optional

max-sixty
max-sixty reviewed Oct 29, 2019
View reviewed changes
xarray/core/computation.py Outdated
@@ -1141,7 +1150,9 @@ def dot(*arrays, dims=None, **kwargs):
einsum_axes = "abcdefghijklmnopqrstuvwxyz"
dim_map = {d: einsum_axes[i] for i, d in enumerate(all_dims)}

if dims is None:
if dims is ALL_DIMS:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if dims is ALL_DIMS:
if dims is ...:

max-sixty
max-sixty reviewed Oct 29, 2019
View reviewed changes
xarray/core/dataarray.py Outdated
@@ -2747,7 +2747,7 @@ def dot(
----------
other : DataArray
The other array with which the dot product is performed.
dims: hashable or sequence of hashables, optional
dims: xarray.ALL_DIMS, hashable or sequence of hashables, optional
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dims: xarray.ALL_DIMS, hashable or sequence of hashables, optional
dims: '...', hashable or sequence of hashables, optional

@max-sixty
Copy link
Collaborator

@mathause I did change the docstrings to .... I tried to commit but don't think I have permissions to your branch

Could we resolve the whatsnew conflicts and then we can merge?

Thank you!

@max-sixty max-sixty merged commit 4d5237b into pydata:master Oct 29, 2019
@max-sixty
Copy link
Collaborator

Super, thanks @mathause !

dcherian added a commit to dcherian/xarray that referenced this pull request Nov 4, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into interp-na-maxgap
ac7aeeb 
* upstream/master:
  __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)
  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)
dcherian added a commit to dcherian/xarray that referenced this pull request Nov 8, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into fix/allow_lazy
6a6ba1b 
* 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)
  ...
@mathause mathause deleted the feature/dot_ALL_DIMS branch August 19, 2020 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-sixty max-sixty max-sixty left review comments

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

Successfully merging this pull request may close these issues.

add ALL_DIMS in xr.dot
4 participants
@mathause @pep8speaks @shoyer @max-sixty