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

        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 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!

Ensure only pandas objects (Series, DataFrame) have their .values extracted during assignment to avoid incorrect coercion of arbitrary objects with a 'values' property (see pydata#1746). Adds regression tests to verify that objects with a values property (e.g. custom classes) are stored as-is in object dtype arrays.

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

srodif mentioned this pull request Aug 31, 2025

Variable.__setitem__ coercing types on objects with a values property srodif/swe-bench-verified-pydata__xarray-2905#1

Open

Copilot AI mentioned this pull request Aug 31, 2025

Fix Variable.__setitem__ coercing types on objects with a values property srodif/swe-bench-verified-pydata__xarray-2905#2

Draft

rowan-stein mentioned this pull request Dec 25, 2025

Variable.__setitem__ coercing types on objects with a values property (Task ID: 298) agyn-sandbox/xarray#7

Open

Uh oh!

Fix in vectorized item assignment #1746

Fix in vectorized item assignment #1746

Uh oh!

Conversation

fujiisoup commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fujiisoup commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fujiisoup commented Dec 6, 2017

Uh oh!

shoyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fujiisoup commented Dec 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoyer commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fujiisoup commented Nov 29, 2017 •

edited

Loading