Split getitem into several dedicated named functions

[jl-wynen]

Overloaded indexing

We heavily overload __getitem__ to perform a variety of tasks based on the type of its argument. While this can make for a convenient interface in simple cases, it makes the implementation very difficult to understand and check. Further, it also makes it difficult to understand what is happening at the call site. Consider for example this code:

data[x, a:b]

Is this label-based or positional indexing? That is, is x a dimension or a coordinate name? The only way to know is by figuring out the types of a and b which can be tricky.
Similarly:

data[x]

Does this look up a child by name, multiple children by nexus class or does it select a single element by index? Again, we would have to know the type of x.
This is also a problem in Scipp which I think we should address. But I think it is exacerbated in ScippNexus due to the many more overloads (although DataGroup shares some of the extra complexity).

I think this is a comprehensive list: (arguments indicate types, not values)

• select child: d[str]
• select all children with matching class: d[type]
• load all: d[()]
• load all: d[...]
• load a single element (1d): d[int]
• load a range (1d): d[slice[int, int, Any]]
• load a single element (nd): d[str, int]
• load a single element (nd): d[str, Variable]
• load a range (nd): d[str, slice[int, int, Any]]
• load a range by label (nd): d[str, slice[Variable, Variable, Any]]
• combination of above for multiple dims: d[dict[str, int | slice[int, int, Any] | slice[Variable, Variable, Any]]]

Granted, not all overloads are available for both groups and datasets. But each has a nontrivial subset of them.

Proposed solution: split into named functions

Ask yourself this question: How often do we call __getitem__ (outside of ScippNexus) and do not know which of the above cases we want? I would expect this number to be almost if not exactly zero. So why do we have code that has to determine, at run time, which of the options the caller selected?

So here is my proposal: Split up __getitem__ into separate named functions which each handle a single user case. This way, each function becomes much simpler and the ambiguity at the call site disappears. This approach is similar to the sel, isel, etc. functions in Pandas.

• Select a single child
• Select any children by nexus class
• Load dataset (all or sub range by position)
  • I think it is fine to overloade d[()], d[...], d[int], d[slice[int, int, Any]] as that is common enough in Python.
  • While I have come to dislike explicit dim labels in __getitem__ (d[str, int] and d[str, slice[int, int, Any]]), I don't think we can split this off any more, there are too many usages now. So we should stick to it in ScippNexus as well.
• Load all data in a group with a slice
  • I would split this off so that we don't overload d[str] and d[int] etc.
• Load data by label
  • This is a major point of complication in Scipp itself; we should update that as well if we change ScippNexus.
  • In ScippNexus, performance and memory usage of this operation is different from Scipp and it is much more costly than slicing by position. So this should be a clear opt-in from the user.
• Slicing in multiple dimensions
  • Is this feature even used?
  • This does not exist in Scipp, so we don't need to use a syntax here that matches what it would look like in Scipp if we had this feature.

I would not be afraid to break with the analogy with h5py and split d[str] and d[()] into separate functions. Just because they do it does not mean that we have to as well.

I am not proposing function names yet or which operations are covered by named functions and which by __getitem__ because I want to discuss the idea itself first.

[SimonHeybrock]

So here is my proposal: Split up __getitem__ into separate named functions which each handle a single user case. This way, each function becomes much simpler and the ambiguity at the call site disappears. This approach is similar to the sel, isel, etc. functions in Pandas.

* Select a single child

* Select any children by nexus class

* Load dataset (all or sub range by position)
  
  * I think it is fine to overloade `d[()]`, `d[...]`, `d[int]`, `d[slice[int, int, Any]]` as that is common enough in Python.
  * While I have come to dislike explicit dim labels in `__getitem__` (`d[str, int]` and `d[str, slice[int, int, Any]]`), I don't think we can split this off any more, there are too many usages now. So we should stick to it in ScippNexus as well.

I think it is fine to deprecate this and accept a dict instead, d[dict[str, int]] etc.?

* Load all data in a group with a slice
  
  * I would split this off so that we don't overload `d[str]` and `d[int]` etc.

* Load data by label
  
  * This is a major point of complication in Scipp itself; we should update that as well if we change ScippNexus.
  * In ScippNexus, performance and memory usage of this operation is different from Scipp and it is much more costly than slicing by position. So this should be a clear opt-in from the user.

Yes.

* Slicing in multiple dimensions
  
  * Is this feature even used?
  * This does not exist in Scipp, so we don't need to use a syntax here that matches what it would look like in Scipp if we had this feature.

See above, would be solved with dict syntax.

I would not be afraid to break with the analogy with h5py and split d[str] and d[()] into separate functions. Just because they do it does not mean that we have to as well.

Yes, probably.

I am not proposing function names yet or which operations are covered by named functions and which by __getitem__ because I want to discuss the idea itself first.

My initial idea would be to keep __getitem__, but only for positional-dataset loading. Getting a subgroup or label-based-dataset loading would get dedicated names?

[nvaytet]

How often do we call getitem (outside of ScippNexus) and do not know which of the above cases we want? I would expect this number to be almost if not exactly zero.

You usually know what you want to do yes.

So why do we have code that has to determine, at run time, which of the options the caller selected?

I fear that using dedicated functions for everything will mean that users (including myself) would have to remember a (long?) list of different methods that we need to use for different cases. "What is isel or iloc for this case?" And so on.
I already find that remembering the different ones in Pandas is difficult for someone who doesn't use Pandas every day.

Right now, I feel that using the getitem and then scippnexus just figures out what it needs to return (and gets it right almost every time? unless there are cases that I don't know of where it gets it wrong) is nice to use.

We already have the "code that has to determine, at run time, which of the options the caller selected", and it works. I would say that there isn't a massive need to change the entire API at this point?

[jl-wynen]

I fear that using dedicated functions for everything will mean that users (including myself) would have to remember a (long?) list of different methods that we need to use for different cases. "What is isel or iloc for this case?" And so on.
I already find that remembering the different ones in Pandas is difficult for someone who doesn't use Pandas every day.

Now you have to remember several pieces of syntax and intricacies of what different argument types mean and how they relate to each other. Also, function names are searchable and can be auto completed. Indexing syntax can not.

Right now, I feel that using the getitem and then scippnexus just figures out what it needs to return (and gets it right almost every time? unless there are cases that I don't know of where it gets it wrong) is nice to use.

It does for you. I remember struggling a lot with the difference between positional and label based indexing because of the different meanings of the arguments. And I had to explain that to several people. Neither of whom initially knew that label based indexing existed but they ran into strange exceptions.

We already have the "code that has to determine, at run time, which of the options the caller selected", and it works. I would say that there isn't a massive need to change the entire API at this point?

Does it work? I don't feel confident saying that based on how complicated the code is and how many different cases there are to test. We can argue about whether this matters, but type checking for this kind of code is cumbersome because the return value of __getitem__ cannot be used safely without an isinstance check.)

[jl-wynen]

I think it is fine to deprecate this and accept a dict instead, d[dict[str, int]] etc.?

I don't think this would help. It doesn't make handling arguments much easier and it requires uses to type extra curly braces. (Awkward on some keyboards)

[SimonHeybrock]

I don't think there is a real alternative to the dict syntax (unless you prefer some sort of tuple or special helper class)?

I don't think this would help. It doesn't make handling arguments much easier

Does it not? Where lies the complication?

requires uses to type extra curly braces. (Awkward on some keyboards)

Almost no one uses this. Awkward is ok, clarity is more important.

[nvaytet]

but type checking for this kind of code is cumbersome

We probably have different opinions about this, but I feel that type-checking shouldn't be dictating design/UX choices.

[jl-wynen]

requires uses to type extra curly braces. (Awkward on some keyboards)

Almost no one uses this. Awkward is ok, clarity is more important.

Didn't you just suggest to remove d[str, int] and replace it with d[dict[str, int]]? That would mean that everyone uses this.

[jl-wynen]

but type checking for this kind of code is cumbersome

We probably have different opinions about this, but I feel that type-checking shouldn't be dictating design/UX choices.

I see difficulty to type-check as an indicator that the interface is complicated. If it is difficult to explain statically (meaning with types) what a function does, then it is also difficult to understand for humans. You need knowledge from a larger context than the immediate call to understand what operation is being performed. (input types)

Plus, we rely on types to determine what operation to use. So shouldn't we then be able to express this relationship in a simple manner?

[SimonHeybrock]

Can you clarify what you disagreement is about Jan-Lukas and Neil? From my point of view positional indexing, label-based indexing and "changing directory" are three distinct things and are worth splitting. But using, e.g., positional with or without dim is mentally very similar and should imho remain the same method.