Add dict-based indexing to Variable, DataArray, and Dataset (#3711) by doronbehar · Pull Request #3925 · scipp/scipp

doronbehar · 2026-07-06T17:34:52Z

Co-Authored-By: Claude Sonnet 4.6 [email protected]

doronbehar · 2026-07-06T17:36:54Z

OK so CI is red not because of me - dict-getsetitem must be a valid branch name!

jl-wynen

Thank you for this contribution!

jl-wynen · 2026-07-07T05:33:51Z

+    for (std::size_t i = 0; i + 1 < items.size(); ++i) {
+      target = target.attr("__getitem__")(
+          py::make_tuple(items[i].first, items[i].second));
+    }


This loop is on both new functions. Can you extract it?

Looks like the AI resolved this comment and the latter comment about Python casting in 1 shot.

jl-wynen · 2026-07-07T05:36:57Z

+    py::object result = py::cast(self);
+    for (const auto &item : index) {
+      result = result.attr("__getitem__")(
+          py::make_tuple(item.first, item.second));


I'm not too happy with casting to Python types here. Is there a way to do this in C++ directly?

I'm not sure I understand this. TBH I don't understand much about mixing C++ with Python etc. I'm just a scientist who likes Python and likes programming, and I'm paying the tokens for the AI :). Anyway, I gave it your comments and it solved them but I'm not able to vouch for whether this particular comment of yours is resolved properly.

jl-wynen · 2026-07-07T05:38:37Z

OK so CI is red not because of me - dict-getsetitem must be a valid branch name!

Does this happen because you opened this PR from a fork? I guess we need to update our CI config.

doronbehar · 2026-07-07T05:41:59Z

OK so CI is red not because of me - dict-getsetitem must be a valid branch name!

Does this happen because you opened this PR from a fork? I guess we need to update our CI config.

I opened the PR from a fork, because I'm not a member of the scipp organization :).

doronbehar · 2026-07-07T12:15:21Z

All issues above were fixed, but converted to a draft since #3926 is a prerequisite for this PR (this PR's tests to be particular).

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Make the test file name fit other test files\' names - like slicebyvalue_test.py

doronbehar · 2026-07-22T09:54:30Z

PR is rebased and ready for review.

Add strict=True to zip() calls (B905), annotate mutable class attributes with ClassVar (RUF012), and wrap long parametrize decorators (E501). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

SimonHeybrock · 2026-07-22T11:57:34Z

Thanks, starting to look into this now, I may push some changes at some point.

Parse the index dict into C++ slice parameters up front, then apply them with the GIL released. Since dict keys are unique, each dimension is indexed at most once and the result is order-independent, so view-producing slices are applied before integer-array extractions to keep copies small. This fixes a silent no-op in __setitem__: integer-array indexing returns a copy, so assigning through a dict containing an integer array wrote to a temporary. Such indices are now rejected with a TypeError. Further changes: - Accept numpy integer index values, matching tuple-based indexing. - Clear errors for non-string keys and for label-based indexing of Variable. - obj[{}] returns a full-slice view like obj[...], instead of a plain shallow copy. - DataGroup.__getitem__ supports dict indices, since ScippIndex now includes them. - Type stubs: dict __setitem__ accepts only int/slice (plus label Variable for DataArray/Dataset); integer arrays are rejected. Co-Authored-By: Claude Fable 5 <[email protected]>

SimonHeybrock · 2026-07-22T12:13:14Z

Thanks for getting this rolling @doronbehar — the feature is a good fit, and the test coverage you added was a useful base. I've pushed a commit on top of your branch that reworks the internals; since some of it is subtle C++/pybind territory, it seemed more efficient to push the changes than to spell them out in review comments. Summary of what changed and why:

A silent data-loss bug in __setitem__. Integer-array indexing (obj[{'x': [0, 2], ...}]) returns a copy in scipp, not a view. The previous implementation chained __getitem__ for all but the last dict entry, so an integer array anywhere before the last position made the assignment write into a temporary and silently discard the values. Dict-based __setitem__ now rejects integer-array indices with a TypeError (the tuple-based API never supported them in __setitem__ either).

Parse first, then slice. The dict is now parsed into C++ slice parameters up front, and the actual slicing runs with the GIL released, consistent with the other indexing overloads. Because dict keys are unique, each dimension is indexed at most once, so the result is independent of dict order — the docs now say so instead of promising "insertion order". This also lets us apply view-producing slices before the copying integer-array extractions, keeping copies as small as possible. (This addresses @jl-wynen's earlier comment about dispatching on Python types: the per-item py::isinstance/py::cast chains are gone from the hot path, and the label-based __setitem__ special-casing collapsed into the existing slicer::set machinery.)

Edge cases aligned with tuple-based indexing. numpy integer scalars now work as index values (floats are still rejected), non-string keys and label-indexing a plain Variable produce clear errors, and obj[{}] returns a full-slice view like obj[...] rather than a plain shallow copy.

DataGroup and typing. Widening ScippIndex with dict made DataGroup.__getitem__ claim dict support it didn't have, so it now has it. The stubs were narrowed to what __setitem__ actually accepts.

I also flattened the test file (the label-based test class needed a fair bit of Python scoping gymnastics — plain parametrization does the same with less machinery) and extended coverage for the cases above.

Co-Authored-By: Claude Fable 5 <[email protected]>

SimonHeybrock · 2026-07-22T12:39:34Z

@jl-wynen Could you have a look at my changes? Thanks!

doronbehar · 2026-09-02T15:14:22Z

@jl-wynen 🙏

jl-wynen · 2026-09-08T07:01:07Z

+ParsedDictIndex parse_dict_index(T &self, const py::dict &index) {
+  ParsedDictIndex parsed;
+  for (const auto &item : index) {
+    if (!py::isinstance<py::str>(item.first))


This is more restrictive than indexing with tuples. E.g., this works:

x[np.str_('abc'), 0]

but with the proposed code, the following would fail:

x[{np.str_('abc'): 0}]

To fix this, you can use a function like this:

Dim parse_dict_key_as_dim(const py::handle &key) { try { return Dim{py::cast<std::string>(key)}; } catch (const py::cast_error& e) { throw except::TypeError( "Dict-based indexing requires string keys (dimension labels)."); } }

As an added bonus, this gets rid of the duplicate type check.

Good catch on the asymmetry, though the example doesn't quite hold: np.str_ subclasses str, so py::isinstance<py::str> already accepted it. The case that did fail is bytes -- pybind11's std::string caster accepts those, so x[b'abc', 0] worked while x[{b'abc': 0}] raised.

Applied your helper, which removes that divergence along with the duplicate check. Added a test with an np.str_ key anyway, since it's the more likely thing for someone to hit.

jl-wynen · 2026-09-08T07:06:13Z

+      if constexpr (std::is_same_v<T, Variable>)
+        throw except::DimensionError(
+            "Label-based indexing requires coordinates and is not supported "
+            "for Variable. Use a DataArray or positional indices instead.");


Suggested change

"for Variable. Use a DataArray or positional indices instead.");

"for Variable. Use positional indices instead.");

self is a Variable here, suggesting to use a DataArray doesn't make a lot of sense here.

jl-wynen · 2026-09-08T07:07:22Z

+            get_slice_params(self, dim, py::cast<Variable>(val))));
+    } else if (auto arr = try_cast<std::vector<scipp::index>>(val))
+      parsed.int_arrays.emplace_back(dim, std::move(*arr));
+    else if (auto i = try_cast<scipp::index>(val)) // e.g. numpy integers


Do we need the check for py::int_ above? Seems like this branch would handle that case.

No, removed. try_cast<scipp::index> covers plain ints, and pybind11's integer caster rejects floats even with conversion enabled, so {'x': 0.5} still raises.

The one behaviour change is for ints too large for scipp::index: those now give the same TypeError as tuple-based indexing rather than escaping as a cast_error.

jl-wynen · 2026-09-08T07:09:53Z

+        if isinstance(name, dict):
+            out = DataGroup(self)
+            for dim, index in name.items():
+                out = out[dim, index]


This is technically not the same as for variables and data arrays because it does not reorder array indices to the end. I think it only affects performance and makes no difference to the final result. Is that correct?

Almost -- there is one case where it changes the result rather than just performance. Slicing one dimension first can reduce a multi-dimensional coord to 1-D, which makes a label-based lookup along another dimension succeed:

dg[{'y': 0, 'x': sc.scalar(1.0, unit='m')}] # worked dg[{'x': sc.scalar(1.0, unit='m'), 'y': 0}] # DimensionError

DataArray raises for both, since it resolves every index against the unsliced object.

Since dict keys carry no meaningful order, I'd rather remove the order dependence than reorder on top of it. DataGroup now forwards the dict, restricted to the dims each item has, to the item itself, so the C++ path stays the single definition of the semantics and DataGroup inherits the array-last ordering too. Bins keeps the loop as it has no dict overload.

Dict keys were checked with py::isinstance<py::str>, which is stricter than the tuple-based overloads: those cast the first tuple element with pybind11's std::string caster, which also accepts bytes. Casting the key the same way removes the divergence and the duplicate type check. Also drop the py::int_ branch, which try_cast<scipp::index> already covers (pybind11's integer caster rejects floats even with conversion enabled), and stop suggesting DataArray in an error raised for Variable.

A dict index is a mapping, not a sequence of operations, so the result must not depend on the order the keys happen to be written in. DataGroup applied the indices one dimension at a time, which does not give that: slicing one dimension can reduce a multi-dimensional coord to 1-D and thereby make a label-based lookup along another dimension succeed that Variable and DataArray reject. Forward the dict, restricted to the dims an item has, to the item itself. Variable, DataArray and Dataset resolve all indices against the unsliced object, so DataGroup now inherits both the order independence and the ordering of array extraction that keeps copies small. Bins keeps the loop as it supports only tuple-based indexing.

doronbehar · 2026-09-09T11:11:04Z

Thanks a lot @SimonHeybrock !

doronbehar force-pushed the dict-getsetitem branch from 406f7a0 to 749e075 Compare July 6, 2026 17:48

doronbehar mentioned this pull request Jul 6, 2026

Set/Get a value by a dictionary of dimensions names and values #3711

Closed

jl-wynen reviewed Jul 7, 2026

View reviewed changes

doronbehar mentioned this pull request Jul 7, 2026

Dataset: Raise DimensionError like Variable & DataArray when indexed #3926

Merged

doronbehar force-pushed the dict-getsetitem branch from 749e075 to 9a29c39 Compare July 7, 2026 12:14

doronbehar marked this pull request as draft July 7, 2026 12:15

doronbehar and others added 5 commits July 22, 2026 12:53

Add dict-based indexing to Variable, DataArray, and Dataset (scipp#3711)

a976ff6

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

bind_slice_methods: avoid casting python types

da66801

dict_indexing_test.py: test empty dict & that errors are raised

2e3954b

slicebydict_test: rename from dict_indexing_test.py

9f5ce59

Make the test file name fit other test files\' names - like slicebyvalue_test.py

Add TestSliceByLabelDict

703782c

doronbehar force-pushed the dict-getsetitem branch from 0f25bcb to 703782c Compare July 22, 2026 09:53

doronbehar marked this pull request as ready for review July 22, 2026 09:54

slicebydict_test: fix ruff/pycodestyle CI errors

a84e275

Add strict=True to zip() calls (B905), annotate mutable class attributes with ClassVar (RUF012), and wrap long parametrize decorators (E501). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Apply ruff-format to typing.py

e2f8ed4

Co-Authored-By: Claude Fable 5 <[email protected]>

jl-wynen reviewed Sep 8, 2026

View reviewed changes

SimonHeybrock added 2 commits September 8, 2026 10:37

jl-wynen approved these changes Sep 8, 2026

View reviewed changes

Merge branch 'main' into dict-getsetitem

303f7ef

SimonHeybrock enabled auto-merge September 9, 2026 10:27

SimonHeybrock merged commit 12eecb3 into scipp:main Sep 9, 2026
4 checks passed

	"for Variable. Use a DataArray or positional indices instead.");
	"for Variable. Use positional indices instead.");

Conversation

doronbehar commented Jul 6, 2026

Uh oh!

doronbehar commented Jul 6, 2026

Uh oh!

jl-wynen left a comment

Choose a reason for hiding this comment

Uh oh!

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!

jl-wynen commented Jul 7, 2026

Uh oh!

doronbehar commented Jul 7, 2026

Uh oh!

doronbehar commented Jul 7, 2026

Uh oh!

doronbehar commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonHeybrock commented Jul 22, 2026

Uh oh!

SimonHeybrock commented Jul 22, 2026

Uh oh!

SimonHeybrock commented Jul 22, 2026

Uh oh!

doronbehar commented Sep 2, 2026

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!

Uh oh!

doronbehar commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

doronbehar commented Jul 22, 2026 •

edited

Loading