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

This seems correct to me. But I would like to wait for @SimonHeybrock to be back from holiday to see if there was an underlying reason why we return -1 if the dim is not found. Is that ok with you?

Is that ok with you?

If you ask me, I guess yea I won't mind - I'm not in a hurry in general and this PR is only blocking the merge of #3925, not the review of it.

This seems correct to me. But I would like to wait for @SimonHeybrock to be back from holiday to see if there was an underlying reason why we return -1 if the dim is not found. Is that ok with you?

I didn't think there was an underlying reason, Claude confirms:

I found the origin of it. Here's the story:

It just fell out of the implementation — it wasn't a deliberate design choice.

The dim_extent helper was introduced back in commit a1180c2d0 ("Slice assignment and basic math bindings"), accompanied by this comment:

// TODO We really need a way to get the dimension labels and extents from
// Dataset (not using class Dimensions).
template <class T> auto dim_extent(const T &object, const Dim dim) {
  if constexpr (std::is_same_v<T, Dataset> || std::is_same_v<T, DatasetProxy>) {
    scipp::index extent = -1;
    for (const auto & [ key, item ] : object) {
      if (item.dims().contains(dim)) {
        if (extent == -1)
          extent = item.dims()[dim];
        else if (item.dims()[dim] == extent - 1)
          --extent;
      }
    }
    return extent;
  } else {
    return object.dims()[dim];
  }
}

At that point Dataset had no unified way to report a dim's extent (no sizes()), so the code had to loop over the dataset's items and infer the extent from whichever items happened to contain that dim. -1 was just the loop's "not yet initialized" sentinel — it was never meant to be a meaningful "dim not found" signal, it was simply what the variable held if the loop never found the dim in any item.

Later (commit 8af1f6782, "Update Python bindings", May 2021), once Dataset::sizes() existed, the code was simplified to:

scipp::index extent = -1;
if (object.sizes().contains(dim))
  extent = object.sizes().at(dim);
return extent;

This carried the -1 default forward mechanically rather than re-deriving the right behavior — it kept the old sentinel pattern even though the reason for it (needing to scan items) no longer existed. The -1 then leaked out as the return value whenever a dim didn't exist, which fed into pybind11's slice.compute(size, ...) as a bogus size instead of raising, so Dataset.__getitem__/__setitem__ on a nonexistent dim silently produced garbage/empty slices instead of raising DimensionError like Variable and DataArray do.

PR #3926 fixes this by just returning object.sizes()[dim] directly (which throws when the dim is absent), matching the Variable/DataArray branch — so no, the -1 behavior wasn't intentional, it was leftover implementation residue from before Dataset had a proper sizes() API.