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

• Rebase to master once Default dtype of pandas.Series is now a categorical #126 is merged
• Update tests
• Update changelog
• 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: Patch coverage is 89.09091% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.93%. Comparing base (6b37c63) to head (f3bbede).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
yasa/hypno.py	87.50%	3 Missing ⚠️
yasa/staging.py	85.71%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
- Coverage   92.59%   83.93%   -8.66%     
==========================================
  Files          23       24       +1     
  Lines        3104     3536     +432     
==========================================
+ Hits         2874     2968      +94     
- Misses        230      568     +338     

☔ View full report in Codecov by Sentry.
📢 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.

3d5c72e

[remrama]

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

[remrama]

@raphaelvallat I forgot how rowdy this PR is. This Hypnogram class is soooo nice! Great idea and clear implementation.

I think you can merge this as soon as you'd like. Once it is in together with the evaluation module we can start to play around with them simultaneously and work out any kinks there.

My comments are minor, though I'll emphasize that I think the assertion messages are important. I love this new object-oriented approach to hypnograms, but it's also true that many users are transitioning to YASA and Python at the same time, and the object-oriented approach might be a bit more intimidating. Maybe not, but if so, a few helpful messages here-and-there might go a long way.

[remrama]

Right after checking that values is a list, I would check they are all strings, with a useful error message telling the use that "yasa expects strings for each epoch now". The first thing I tried when playing around was:

>>> yasa.Hypnogram([1, 2, 3, 4])
# AttributeError: 'int' object has no attribute 'upper'

I'm quite sure this will happen for many users when upgrading to 0.7, and there should probably be as much hand-holding as possible.

[raphaelvallat]

Good idea — added a new check

[remrama]

A similar hand-holding opportunity is here, where I think it'd be nice to inform the user that they need to specify n_stages if they are trying to do specify a hypnogram that has less than 5 possible stages.

>>> yasa.Hypnogram(["S", "W"])
# AssertionError: ['S' 'W'] do not match the accepted values for a 5 stages hypnogram: ['WAKE', 'W', 'N1', 'N2', 'N3', 'REM', 'R', 'ART', 'UNS']

Sidenote, and minor opinion, but I think the wording throughout should be "N-stage hypnogram" instead of "N stages hypnogram". It's varied in a few places, but I think stages should be singular and the hyphen should be there. To me it seems clearer that it's referring to the number of potential stages (which for many sleep researchers, having <5 is not really an obvious need).

[remrama]

Also I think adding the word "unique" to the property string would be helpful: <Hypnogram | 2 epochs x 30s (1.00 minutes), 2 unique stages>

[raphaelvallat]

See edits!

[remrama]

This might not be the best place to comment this, but regarding the proba that is automatically returned from SleepStaging, should this not have a Hynogram.scorer attached to it? Maybe just "YASA" or even something with the version number like "YASA-vX.X"? Or I'm not sure if you have a separate name and/or version control for the underlying sleep stager.

But in any case, I think the usefulness of the scorer attribute is that it allows for convenient comparisons, especially if someone is saving their data and looking back at it after a new stager comes out. Note also that the evaluation module plotting functions often take advantage of this attribute for plotting and dataframe organization, so using it in the returned dataframe might help out in those instances too.

[raphaelvallat]

Hmm, I like the idea but what would be the best way to attach such an attribute to a pandas DataFrame? In any cases, I think this can be done in a separate PR.

[remrama]

I'm not sure what causes this, but note the differences in iPython:

In: pd.read_csv
Out: <function pandas.io.parsers.readers.read_csv(filepath_or_buffer: 'FilePath ...

In: h.as_int
Out:
<bound method Hypnogram.as_int of <Hypnogram | 3 epochs x 30s (1.50 minutes), 3 stages>
 - Use `.hypno` to get the string values as a pandas.Series
 - Use `.as_int()` to get the integer values as a pandas.Series
 - Use `.plot_hypnogram()` to plot the hypnogram
See the online documentation for more details.>

I often hit enter on methods without calling them, just to get a quick (if ugly) view of the arguments and such. When I do this on the Hypnogram methods it just returns the object string representation. I'm not sure what is necessary to switch it over.

[raphaelvallat]

I think you just need to add a "?" at the end 👇

[remrama]

The MNE annotations glossary link in the docstrings is dead now. And that actually highlights an anxiety of mine about this nice and convenient as_annotations method; I really like using the BIDS standards for events files, but to-date I've struggled with comprehending the terminology in MNE between events and annotations. They have events_to_annotations and annotations_to_events methods, and I'm sure that if I look at the docs today there will be a clear differentiation, but as I recall I am consistently "confident-and-subsequently-confused" about how they handle this.

So I think this method is useful but I don't know about the name of it. Maybe it's just as_events for now? Because that is also consistent with BIDS, which is just looking for an events dataframe, and that's what this is.

[raphaelvallat]

Agreed — renamed to as_events

[raphaelvallat]

@remrama thanks for the great comments! I believe this PR is now ready to be merged :) Don't worry about the linting/formatting errors since these will be fixed with #187