Support __getitem__ on the bins property for more concise value/interval extraction
#2831
Conversation
src/scipp/core/bins.py
Outdated
| dim, index = key | ||
| if isinstance(index, _cpp.Variable): | ||
| if index.ndim == 0: | ||
| return self._obj.group(concat([index], dim)).squeeze(dim) |
There was a problem hiding this comment.
I think this would be clearer if it used broadcast instead of concat.
There was a problem hiding this comment.
I had an issue with that as for some reason the C++ bin implementation does not work as it thinks it is operating on a non-contiguous object.
There was a problem hiding this comment.
Hmm, I thought I should be able to use flatten, but for 0-D it falls back to broadcast, which means it will also think it is non-contiguous (i.e., has a stride of 0). Maybe we should fix flatten to return something with stride 1 (of length 1) instead?
There was a problem hiding this comment.
Done, please have another look @jl-wynen
There was a problem hiding this comment.
This now means that the variable can have any dimensions as long as it has only a single element. This is inconsistent with value based slicing on the data array itself and I don't think we should allow it.
There was a problem hiding this comment.
Which variable? The key is checked for ndim==0.
| raise ValueError( | ||
| f"Unsupported key '{key}'. Expected a dimension label and " | ||
| "a 0-D variable or a dimension label and a slice object with start " | ||
| "and stop given by a 0-D variable.") |
There was a problem hiding this comment.
You can add a type annotation for key to make this a bit clearer.
This addresses an aspect of the long standing issue scipp/scippneutron#237 (comment), in particular cases a) and c). After this change, I believe we are in a position to update/rewrite the filtering documentation and finally close that issue.