SleepStaging returns a yasa.Hypnogram instance

[raphaelvallat]

Work in progress / Do not review

This PR changes the return type of SleepStaging.predict() from a numpy.array to the newly-created yasa.Hypnogram class.

Remaining tasks

• [x] Rebase to master once #126 is merged
• [x] Update tests
• [x] Update changelog
• [x] Update string representation of SleepStaging
• [ ] Update FAQ and quickstart
• [ ] ~Warning in SleepStaging.predict that the hypnogram values have changed, specifically: "W" -> "WAKE", "R" -> "REM"!~
• [ ] Add example on how to add as an MNE.Annotations

[remrama]

unless we decide to switch to "W" and "R" as the default string in yasa.Hypnogram

I think it should stay as full spellings WAKE and REM, so SleepStaging should conform to Hypnogram. I see no huge benefit to abbreviations, and the full spellings are clearer, especially in the lower n_stages sitatuations (and should be consistent across).

Add example on how to add as an MNE.Annotations

Do you think it's possible to add a probabilities attribute to yasa.Hypnogram? As opposed to returning them separately? This would be very useful for the upcoming evaluation module and some plotting. They will both need probabilities so I see no reason to not attach them here. Plus, I think probability estimates are going to become kind of the "norm" for hypnograms moving forward, so people will want to work with them frequently. One could even make a yasa.Hypnogram with probabilities derived from multiple human scorers. Seems useful.

Of course in this case they could just be added to as_annotations output, maybe with an include_probabilities boolean argument.

[raphaelvallat]

SleepStaging should conform to Hypnogram

Good point. I'll make the change.

Do you think it's possible to add a probabilities attribute to yasa.Hypnogram?

Yep, also a good idea. There's going to be some tricky edge cases. For example, when using upsample, should we also upsample and interpolate the proba? Or just pass proba=None. Also, should we move the SleepStaging.plot_predict_proba to yasa.Hypnogram.plot_proba instead. But regardess of these questions I can include in this PR a simple implementation of a Hypnogram(..., proba=None) parameter

[codecov-commenter]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (6b37c63) 92.59% compared to head (3f92f8b) 92.63%.

Files	Patch %	Lines
yasa/staging.py	83.33%	4 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #127 +/- ## ========================================== + Coverage 92.59% 92.63% +0.04% ========================================== Files 23 23 Lines 3104 3136 +32 ========================================== + Hits 2874 2905 +31 - Misses 230 231 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[remrama]

should we also upsample and interpolate the proba?

Oooh, you're right, that's a weird one. I think passing None is reasonable (definitely for now, but probably forever too). I don't imagine there are many situations where someone needs the probabilities for the upsampled data?? 🤔

Also, should we move the SleepStaging.plot_predict_proba to yasa.Hypnogram.plot_proba instead?

Yes definitely. As you said, keep it where it is for now. I can submit a separate PR for that later.

[remrama]

@raphaelvallat I don't want to throw any wrenches in at this point, but I just considered this. Maybe you can tell me why it's a bad idea?

If users aren't going to need the yasa.SleepStaging.predict_probas and plot_probas methods, is there a reason to even work with the yasa.SleepStaging class instance directly? I mean, would it be simpler (with no cost) to just have a yasa.predict_hypnogram function that uses the SleepStaging class under-the-hood to return a yasa.Hypnogram?

I'm asking now, because if we liked this, then this would be a nice time to implement it without breaking the API, because you could actually keep the normal behavior of SleepStaging.predict to return a numpy array, then just create the Hypnogram instance in the predict_hypnogram function. I imagine you could always keep the SleepStaging interface, for advanced users who want the .get_features attributes or something.

[raphaelvallat]

It's not a bad idea at all. I'll have to think more about it. My initial preference would be to vote no, mostly because I'm being lazy and because I will have much less time to work on YASA in the coming weeks, but also:

• yasa.SleepStaging class is probably the most widely-used module in YASA and I don't want to remove it. I think the flexibility of the (explicit) class implementation is a big advantage. I know quite a lot of people use the get_features method too.
• I don't want to deprecate predict_proba and plot_predict_proba right away. I guess more generally for the next release I don't want to make big changes to the SleepStaging module. Changing the return type of predict to a yasa.Hypnogram instance is already going to be a hassle for most users.

That said, I'm not against implementing the shortcut yasa.predict_hypnogram in a future release, as long as we don't deprecate the SleepStaging class.

[raphaelvallat]

@remrama PR ready for review! I still need to update the changelog and FAQ but I think this can be done in a final PR before we release v0.7 — together with some cleaning of the existing notebooks. Also, if we decide to switch to Pooch than most of the examples might change anyway.

[raphaelvallat]

Btw I'm removing the FutureWarning in yasa.hypno_int_to_str. I think a lot of users are going to use it to convert their integer hypnograms to a yasa.Hypnogram instance and it's annoying to have a warning every time. I was actually in that situation just a few days ago when updating some of my code.

https://github.com/raphaelvallat/yasa/pull/127/commits/3d5c72e06feae638dbec18538be59b94561a2fbb

[remrama]

I'm moving to this after #130 is finalized. Then we can swap reviews 🎉