Enable setitem for dask versions that support it. #5174

tammasloughran · 2021-04-16T18:33:28Z

The basics seem to work for me with just these changes. But I haven't tested much to see if it plays nicely with the rest of xarray. Looks like dask's item assignment is still mostly limited to 1D arrays and slices of multi dimensional arrays—no fancy stuff.

pep8speaks · 2021-04-16T18:33:33Z

Hello @tammasloughran! 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 2021-05-01 11:11:23 UTC

xarray/core/indexing.py

max-sixty · 2021-04-16T21:04:52Z

Looks good! Not sure whether there's a reasonable way of testing this...

Are there any current tests for the error that break on the new version (i.e. because it doesn't raise an error)?

tammasloughran · 2021-04-19T11:01:33Z

Are there any current tests for the error that break on the new version (i.e. because it doesn't raise an error)?

I don't quite understand understand what you mean. Like this?

max-sixty · 2021-04-19T13:19:05Z

I don't quite understand understand what you mean. Like this?

Forgive me not being clear. The current test with the version check is great. And I hadn't realized there was a released dask version — even better that the test is already running on both versions of the code.

xarray/tests/test_indexing.py

xarray/core/indexing.py

xarray/tests/test_indexing.py

dcherian · 2021-04-19T17:48:56Z

Thanks @tammasloughran This is a great PR!

tammasloughran · 2021-04-19T18:56:27Z

I don't quite understand understand what you mean. Like this?

Forgive me not being clear. The current test with the version check is great. And I hadn't realized there was a released dask version — even better that the test is already running on both versions of the code.

Well... I see now there is also a duplicate test here, which fails when using the newer dask version. I don't understand why xarray goes to all the trouble of directly unit testing an external library. Makes more sense to me to only test the xarray wrapper for the dask functionality. I could add another version check there, or remove that test. What do you think?

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

dcherian · 2021-04-19T19:07:05Z

I see now there is also a duplicate test here, which fails when using the newer dask version. I

That's testing the Variable object which is the lowest-level Xarray object. It would be a good place to add the assignment tests

xarray/core/indexing.py

…e index.

xarray/tests/test_dask.py

andersy005

@tammasloughran, I left a few inline suggestions

xarray/core/indexing.py

max-sixty · 2021-04-21T17:43:51Z

After those two suggestions, let's merge?

Co-authored-by: Anderson Banihirwe <axbanihirwe@ualr.edu>

… version to '0' if dask is not installed.

max-sixty · 2021-04-30T17:13:07Z

@tammasloughran would be great to get this in by #5232 if you have any time, it's great work. I think it's fairly close to the tests passing.

xarray/core/indexing.py

Illviljan · 2021-04-30T18:40:58Z

The errors seems unrelated? Seems to rather be a mismatch in the CI?

A dask method __dask_distributed_pack__ is complaining:

>       from distributed.protocol.core import dumps_msgpack
E       ImportError: cannot import name 'dumps_msgpack' from 'distributed.protocol.core' (/usr/share/miniconda/envs/xarray-tests/lib

But those seems to have been removed in dask/distributed#4677. Further reading in dask/dask-yarn#147 (comment)

Co-authored-by: Illviljan <14371165+Illviljan@users.noreply.github.com>

keewis · 2021-04-30T21:05:09Z

A dask method __dask_distributed_pack__ is complaining:

that seems to have been a conda issue (but it's gone now), and the remaining failure is a known flaky test, so CI looks good

andersy005 · 2021-04-30T21:28:26Z

xarray/tests/test_dask.py

+        if LooseVersion(dask.__version__) >= LooseVersion("2021.04.0+17"):
+            arr = Variable(("x"), da.array([1, 2, 3, 4]))
+            expected = Variable(("x"), da.array([99, 2, 3, 4]))
+            arr[0] = 99  # Indexing by integers
+            assert_identical(arr, expected)
+            arr = Variable(("x"), da.array([1, 2, 3, 4]))
+            expected = Variable(("x"), da.array([99, 99, 99, 4]))
+            arr[2::-1] = 99  # Indexing by slices
+            assert_identical(arr, expected)
+            arr = Variable(("x"), da.array([1, 2, 3, 4]))
+            expected = Variable(("x"), da.array([99, 99, 3, 99]))
+            arr[[0, -1, 1]] = 99  # Indexing by a list of integers
+            assert_identical(arr, expected)
+            arr = Variable(("x"), da.array([1, 2, 3, 4]))
+            expected = Variable(("x"), da.array([99, 99, 99, 4]))
+            arr[np.arange(3)] = 99  # Indexing by a 1-d numpy array of integers
+            assert_identical(arr, expected)
+            arr = Variable(("x"), da.array([1, 2, 3, 4]))
+            expected = Variable(("x"), da.array([1, 99, 99, 99]))
+            arr[[False, True, True, True]] = 99  # Indexing by a list of booleans
+            assert_identical(arr, expected)
+            arr = Variable(("x"), da.array([1, 2, 3, 4]))
+            expected = Variable(("x"), da.array([1, 99, 99, 99]))
+            arr[np.arange(4) > 0] = 99  # Indexing by a 1-d numpy array of booleans
+            assert_identical(arr, expected)
+            arr = Variable(("x"), da.array([1, 2, 3, 4]))
+            expected = Variable(("x"), da.array([99, 99, 99, 99]))
+            arr[arr > 0] = 99  # Indexing by one broadcastable Array of booleans
+            assert_identical(arr, expected)
+        else:
+            with pytest.raises(TypeError, match=r"stored in a dask array"):
+                v[:1] = 0


Nitpick: I was wondering if we could use @pytest.mark.parametrize here to parametrize this test so as to avoid code duplication?

I definitely agree, but I think I'd prioritize merging quickly. For a first PR, it's great code overall. We could add a TODO to the current code so it doesn't look like best practice to the next person.

I concur...

max-sixty · 2021-04-30T22:27:23Z

@tammasloughran this just needs a whatsnew now!

max-sixty · 2021-05-01T19:59:31Z

Excellent — thank you @tammasloughran !

Enable __setitem__ for dask versions that support it.

14d18a7

Cleanup

2e6b196

tammasloughran mentioned this pull request Apr 16, 2021

Dask now supports item assignment. Shouldn't xarray support it too? #5171

Closed

andersy005 reviewed Apr 16, 2021

View reviewed changes

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

Dask is soft dependency

54aaa73

max-sixty reviewed Apr 16, 2021

View reviewed changes

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

tammasloughran added 2 commits April 19, 2021 12:49

Test for item assignment in dask

6ddb76c

Fix minor mistakes

d75cd17

raises_regex typo

1bddcdd

max-sixty reviewed Apr 19, 2021

View reviewed changes

xarray/tests/test_indexing.py Outdated Show resolved Hide resolved

max-sixty reviewed Apr 19, 2021

View reviewed changes

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

dcherian reviewed Apr 19, 2021

View reviewed changes

xarray/tests/test_indexing.py Outdated Show resolved Hide resolved

xarray/tests/test_indexing.py Outdated Show resolved Hide resolved

xarray/tests/test_indexing.py Outdated Show resolved Hide resolved

Use assert_identical instead of assert

0b5c7ad

Update exception text

e725143

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

tammasloughran added 4 commits April 19, 2021 22:14

tests/test_dask.py

c84f774

Moved indexing tests to test_dask.py

f7d7bc4

Code cleanup

63b4429

More cleanup

b9768ce

shoyer requested changes Apr 19, 2021

View reviewed changes

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

tammasloughran added 2 commits April 19, 2021 23:03

OuterIndexer not fully supported by dask. only use one array non-slic…

777faaa

…e index.

Cleanup again

b4cb7d9

tammasloughran requested a review from shoyer April 19, 2021 21:08

Removed duplicate line

b9a697e

tammasloughran and others added 5 commits April 20, 2021 12:02

Conflict correction

d141304

Merge branch 'master' into master

8533072

Style corrections

3b1dbd3

Merge branch 'master' of https://github.com/tammasloughran/xarray

bedfdf7

pytest.raises not raises_regex

c169fd0

max-sixty reviewed Apr 20, 2021

View reviewed changes

xarray/tests/test_dask.py Outdated Show resolved Hide resolved

andersy005 reviewed Apr 21, 2021

View reviewed changes

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

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

tammasloughran and others added 4 commits April 21, 2021 23:02

Update xarray/core/indexing.py

ee6efd0

Co-authored-by: Anderson Banihirwe <axbanihirwe@ualr.edu>

Update xarray/core/indexing.py

9bc9b63

Co-authored-by: Anderson Banihirwe <axbanihirwe@ualr.edu>

New line

ee11d4e

mypy complains of type inconsistency for DASK_VERSION = None. Setting…

ca84825

… version to '0' if dask is not installed.

Illviljan reviewed Apr 30, 2021

View reviewed changes

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

Change ' to "

ae2a21f

Co-authored-by: Illviljan <14371165+Illviljan@users.noreply.github.com>

andersy005 reviewed Apr 30, 2021

View reviewed changes

tammasloughran and others added 2 commits May 1, 2021 13:09

Added TODO and whats-new

73bbadc

Merge branch 'master' into master

153a7f3

max-sixty merged commit 5a0528b into pydata:master May 1, 2021

andersy005 mentioned this pull request May 2, 2021

Parametrize test for __setitem__ (for dask array) #5243

Merged

2 tasks

tomvothecoder mentioned this pull request Oct 27, 2021

Fix NotImplementedError: xarray can't set arrays with multiple array indices to dask yet. xCDAT/xcdat#115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable setitem for dask versions that support it. #5174

Enable setitem for dask versions that support it. #5174

tammasloughran commented Apr 16, 2021

pep8speaks commented Apr 16, 2021 •

edited

Loading

max-sixty commented Apr 16, 2021

tammasloughran commented Apr 19, 2021

max-sixty commented Apr 19, 2021

dcherian commented Apr 19, 2021

tammasloughran commented Apr 19, 2021 •

edited

Loading

dcherian commented Apr 19, 2021

andersy005 left a comment

max-sixty commented Apr 21, 2021

max-sixty commented Apr 30, 2021

Illviljan commented Apr 30, 2021

keewis commented Apr 30, 2021

andersy005 Apr 30, 2021

max-sixty Apr 30, 2021

andersy005 Apr 30, 2021

max-sixty commented Apr 30, 2021

max-sixty commented May 1, 2021

Enable __setitem__ for dask versions that support it. #5174

Enable __setitem__ for dask versions that support it. #5174

Conversation

tammasloughran commented Apr 16, 2021

pep8speaks commented Apr 16, 2021 • edited Loading

Comment last updated at 2021-05-01 11:11:23 UTC

max-sixty commented Apr 16, 2021

tammasloughran commented Apr 19, 2021

max-sixty commented Apr 19, 2021

dcherian commented Apr 19, 2021

tammasloughran commented Apr 19, 2021 • edited Loading

dcherian commented Apr 19, 2021

andersy005 left a comment

Choose a reason for hiding this comment

max-sixty commented Apr 21, 2021

max-sixty commented Apr 30, 2021

Illviljan commented Apr 30, 2021

keewis commented Apr 30, 2021

andersy005 Apr 30, 2021

Choose a reason for hiding this comment

max-sixty Apr 30, 2021

Choose a reason for hiding this comment

andersy005 Apr 30, 2021

Choose a reason for hiding this comment

max-sixty commented Apr 30, 2021

max-sixty commented May 1, 2021

Enable setitem for dask versions that support it. #5174

Enable setitem for dask versions that support it. #5174

pep8speaks commented Apr 16, 2021 •

edited

Loading

tammasloughran commented Apr 19, 2021 •

edited

Loading