Fix in vectorized item assignment #1746

fujiisoup · 2017-11-29T00:37:41Z

Closes Assigning data to vector-indexed data doesn't seem to work #1743, Assignment with vectorized indexing doesn't work with a scalar right-hand-side #1744
Tests added / passed
Passes git diff upstream/master **/*py | flake8 --diff
Fully documented, including whats-new.rst for all changes and api.rst for new API

Found bugs in nputils.NumpyVindexAdapter.__setitem__ and DataArray.__setitem__.

I will add more tests later.
Test case suggestions would be appreciated.

max-sixty · 2017-11-29T04:15:54Z

xarray/tests/test_dataarray.py

+        ind = Variable(['a'], [0, 1])
+        da[dict(x=ind, y=ind)] = 0
+        expected = DataArray([[0, 1], [1, 0], [1, 1]], dims=['x', 'y'])
+        self.assertDataArrayIdentical(expected, da)


For the future (no need to change this time), you can use assert_identical, from the newer testing tools

I always forget this, maybe because I usually make tests based on the existing ones...
(I think it is what many contributors do).
I think we should update entire test cases at some point (similar to #1741), rather than gradual migration.

shoyer · 2017-11-29T10:32:37Z

xarray/core/nputils.py

-        mixed_positions, vindex_positions = _advanced_indexer_subspaces(key)
-        self._array[key] = np.moveaxis(value, vindex_positions,
-                                       mixed_positions)
+        if np.isscalar(value):


I would suggest using utils.is_scalar() and utils.to_0d_array for coercing a scalar value to a numpy array, then using the original code path here.

I just noticed that here we should support broadcasting, rather than some special care of scalars.

fujiisoup · 2017-11-29T14:13:58Z

xarray/core/variable.py

-                value = Variable(dims[-value.ndim:], value).set_dims(dims).data
+                value = Variable(dims[-value.ndim:], value)
+        # broadcast to become assignable
+        value = value.set_dims(dims).data


I decided to revert nputils.NumpyVindexAdapter to the master version and to make a broadcasting here.

fujiisoup · 2017-11-29T14:33:27Z

Tests are failing due to #1738.
I think this is ready for review.

shoyer · 2017-12-04T06:27:38Z

xarray/core/dataarray.py

@@ -484,7 +484,9 @@ def __setitem__(self, key, value):
        if isinstance(key, basestring):
            self.coords[key] = value
        else:
-            # xarray-style array indexing
+            # DataArray key -> Variable key
+            key = {k: v.variable if isinstance(v, DataArray) else v


Do we want to enforce consistency for coordinates here? My inclination would be that we should support exactly the same keys in setitem as are valid in getitem. Ideally we should also reuse the same code. That means we should raise errors if there are multiple indexers with inconsistent alignment.

My inclination would be that we should support exactly the same keys in setitem as are valid in getitem.

Reasonable. I will add a validation.

shoyer · 2017-12-04T06:30:46Z

xarray/core/variable.py

-        if new_order:
-            value = duck_array_ops.asarray(value)
+        if not isinstance(value, Variable):
+            value = as_compatible_data(value)
            if value.ndim > len(dims):
                raise ValueError(


Do we still need this special case error message now that we call set_dims below?

I think value = Variable(dims[-value.ndim:], value) fails if value.ndim > len(dims).

shoyer · 2017-12-06T06:49:15Z

xarray/core/dataarray.py

+            # Coordinates in key, value and self[key] should be consistent.
+            obj = self[key]
+            if isinstance(value, DataArray):
+                assert_coordinate_consistent(value, obj.coords)


I was actually thinking of checking the consistency of coords on each DataArray argument in key instead. I guess we should probably check both!

shoyer · 2017-12-06T06:50:14Z

xarray/core/dataarray.py

+            # Coordinates in key, value and self[key] should be consistent.
+            obj = self[key]
+            if isinstance(value, DataArray):
+                assert_coordinate_consistent(value, obj.coords)


I think if you use obj.coords.variables here we can skip the awkward getattr(coords[k], 'variable', coords[k]) above.

shoyer · 2017-12-06T06:50:41Z

xarray/core/coordinates.py

+    for k in obj.dims:
+        # make sure there are no conflict in dimension coordinates
+        if (k in coords and k in obj.coords):
+            coord = getattr(coords[k], 'variable', coords[k])  # Variable


It would be better to insist that coords always has the same type (e.g., a dict of with Variable values).

That's really reasonable. Updated.

fujiisoup · 2017-12-06T07:30:53Z

I will update docs as this is a backward-incompatible change.

shoyer

Do we also need to update assignment for .loc, or does this already handle it? It would be good to at least add tests. We can leave that for another PR if you prefer.

shoyer · 2017-12-06T18:59:42Z

doc/indexing.rst

+.. note::
+
+  If an indexer is a :py:meth:`~xarray.DataArray`, its coordinates should not
+  conflict with the selected subpart of the target array.


We should probably note the exception for explicitly indexed dimensions with .loc/.sel?

shoyer · 2017-12-06T19:00:54Z

xarray/core/coordinates.py

+    """
+    for k in obj.dims:
+        # make sure there are no conflict in dimension coordinates
+        if (k in coords and k in obj.coords):


nit: you can drop the extra parentheses here inside if

shoyer · 2017-12-06T19:01:40Z

xarray/core/dataarray.py

+                assert_coordinate_consistent(value, obj.coords.variables)
+            # DataArray key -> Variable key
+            key = {k: v.variable if isinstance(v, DataArray) else v
+                   for k, v in self._item_key_to_dict(key).items()}


Do we need to check that coordinates are consistent on the key?

This is done a few lines above, obj = self[key], where in .isel we check the coordinates in the key.

But I am wondering this unnecessary indexing, though I think this implementation is the simplest.

Hmm. This could indeed be a significant performance hit. That said, I'm OK leaving this for now, with a TODO note to optimize it later.

fujiisoup · 2017-12-08T01:49:53Z

@shoyer Thanks for the review.

Do we also need to update assignment for .loc, or does this already handle it?

Done.

As I commented above, coordinate validation requires an (practically) unused indexing.
I am wondering the efficiency of the indexing, though __setitem__ might be less frequently used than __getitem__.

shoyer · 2017-12-08T02:02:13Z

xarray/core/dataset.py

-                pos_indexers[k] = DataArray(pos_indexers[k],
-                                            coords=coords, dims=v.dims)
+        pos_indexers, new_indexes = remap_label_indexers(
+                                        self, method, tolerance, **indexers)


only indent by one additional tab for continuation lines.

shoyer · 2017-12-08T02:04:38Z

xarray/core/dataarray.py

+                assert_coordinate_consistent(value, obj.coords.variables)
+            # DataArray key -> Variable key
+            key = {k: v.variable if isinstance(v, DataArray) else v
+                   for k, v in self._item_key_to_dict(key).items()}


Hmm. This could indeed be a significant performance hit. That said, I'm OK leaving this for now, with a TODO note to optimize it later.

shoyer · 2017-12-08T22:37:51Z

Looks good to me. @fujiisoup please hit the big green button when you're ready!

fujiisoup added 2 commits November 29, 2017 09:24

Fix in NumpyVindexAdapter.__setitem__ and DataArray.__setitem__

9eafe36

Update what's new

2aff4ce

max-sixty reviewed Nov 29, 2017

View reviewed changes

shoyer reviewed Nov 29, 2017

View reviewed changes

fujiisoup added 2 commits November 29, 2017 23:02

Broadcasting in setitem

22c1295

Small clean up. Revert unintended change.

dc135cd

fujiisoup commented Nov 29, 2017

View reviewed changes

fujiisoup changed the title ~~[WIP] Fix in vectorized item assignment~~ Fix in vectorized item assignment Nov 29, 2017

Merge branch 'master' into vindex_assign_patch

b1dd0f0

shoyer reviewed Dec 4, 2017

View reviewed changes

Check coordinate consistency for DataArray.__setitem__

c74c828

shoyer reviewed Dec 6, 2017

View reviewed changes

Only pass a dict of variables to assert_coordinate_consistent

8e6e2e0

Update docs.

aef3d56

shoyer reviewed Dec 6, 2017

View reviewed changes

fujiisoup added 5 commits December 8, 2017 10:00

still working

f10ecf4

Coordinate validation in .loc.__setitem__

d482f80

remove extra parenthesis

c2b5ac3

Refactoring. remap_label_indexers is moved to coordinates.py

84e5e6f

Added an exception in doc for coordinate confliction

2011140

shoyer reviewed Dec 8, 2017

View reviewed changes

Added a TODO for unused indexing part in Loc.__setitem__

6906eeb

fujiisoup merged commit 5e80189 into pydata:master Dec 9, 2017

This was referenced Dec 9, 2017

Assignment with vectorized indexing doesn't work with a scalar right-hand-side #1744

Closed

Needs performance check / improvements in value assignment of DataArray #1771

Open

chstan mentioned this pull request May 1, 2018

Variable.__setitem__ coercing types on objects with a values property #2097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix in vectorized item assignment #1746

Fix in vectorized item assignment #1746

fujiisoup commented Nov 29, 2017 •

edited

Loading

max-sixty Nov 29, 2017

fujiisoup Nov 29, 2017

shoyer Nov 29, 2017

fujiisoup Nov 29, 2017

fujiisoup Nov 29, 2017

fujiisoup commented Nov 29, 2017

shoyer Dec 4, 2017

fujiisoup Dec 4, 2017

shoyer Dec 4, 2017

fujiisoup Dec 4, 2017

shoyer Dec 6, 2017

shoyer Dec 6, 2017

shoyer Dec 6, 2017

fujiisoup Dec 6, 2017

fujiisoup commented Dec 6, 2017

shoyer left a comment

shoyer Dec 6, 2017

shoyer Dec 6, 2017

shoyer Dec 6, 2017

fujiisoup Dec 8, 2017

shoyer Dec 8, 2017

fujiisoup commented Dec 8, 2017

shoyer Dec 8, 2017

shoyer Dec 8, 2017

shoyer commented Dec 8, 2017

Fix in vectorized item assignment #1746

Fix in vectorized item assignment #1746

Conversation

fujiisoup commented Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Dec 6, 2017

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Dec 8, 2017

fujiisoup commented Nov 29, 2017 •

edited

Loading