Demo & RFC: Add return type hints to read_evokeds()

[hoechenberger]

x-ref #12243

Here I'm demonstrating a slightly tricky case for type hints: the value of a parameter determines the function's return value. I just wanted to show to you what this looks like, so we can evaluate whether this is acceptable.

MWE:

import mne

evokeds_path = (
    mne.datasets.sample.data_path() / "MEG" / "sample" / "sample_audvis-ave.fif"
)

evokeds = mne.read_evokeds(evokeds_path)
evoked = mne.read_evokeds(evokeds_path, condition=0)

main:

This PR:

cc @cbrnr @agramfort @larsoner @drammock

[hoechenberger]

Please note that I added 2 commits here. The second one uses an overload for condition: Literal[None], which is very explicit but may be considered unnecessarily verbose. The first one only uses condition: None.

[hoechenberger]

This is how VS Code now presents the signature:

[cbrnr]

Although such cases will hopefully be rare, this example shows that our API should probably be simplified. Why don't we take this opportunity to e.g. always return a list[Evoked]?

[hoechenberger]

I agree that this particular overload may be indicative of an API design we may want to reconsider.

[larsoner]

-1, not worth the hassle to end users

[hoechenberger]

I think the UX would be greatly improved and from a user's perspective, is not different from the example with read_raw() that was generally agreed upon as being convincing.

Right now when I read evokeds, my IDE doesn't know which data type is actually being returned and I have to manually insert type guards in my code.

I tend to agree with @cbrnr that the "hassle" we're looking at here (overloads) might just be indicative of a sub-optimal API design in the first place

[larsoner]

Agreed it's suboptimal, but to me the code churn we will force upon people isn't worth it

[hoechenberger]

The simplified solution for typing this function would be to annotate the return type as the union of Evoked and list of Evoked. At least this would make things clearer when viewing the signature in an IDE.

No overloads required.

Thoughts?

[larsoner]

I don't really understand / haven't used overloads but if you mean having this be the signature:

def read_evokeds(
    ...
) -> Union[Evoked, List[Evoked]]:

then +1 from me, seems simple and reflective of current behavior.

[hoechenberger]

I don't really understand / haven't used overloads

It's just like multiple dispatch in other languages. You specify what goes into the function and what comes out. You repeat that for all input types / parameter values that yield a different function return type. So that in the example above, when you ask the function to read only a single condition, the type checker knows to expect an Evoked; otherwise, a list of Evoked.

The overloads don't change runtime behavior, in case that's something you assumed.

but if you mean having this be the signature:

def read_evokeds(

    ...

) -> Union[Evoked, List[Evoked]]:

then +1 from me, seems simple and reflective of current behavior.

Yes, this is what I meant. It makes the UX slightly better but would still require type guards in user code. Because the return type now is "Evoked or a list of Evoked", hence without inserting an assert isinstance(evoked, ...), you won't get helpful advice from your IDE.

But at least VS Code won't keep telling me that this function returns "Unknown or a list of Unknown" anymore, which is already a slight improvement.