raphaelvallat/yasa

Add Hypnogram.plot_hypnogram method

remrama opened this issue · 6 comments

remrama commented

I'm finalizing a PR that will bring yasa.plot_hypnogram up to speed with the new yasa.Hypnogram class (#116).

I'm only opening this Issue because I'm looking for some feedback before finalizing the PR: Currently, I have yasa.plot_hypnogram requiring a yasa.Hypnogram as input. All the attributes make it way easier to handle all the plotting. Should it also allow a sequence as it used to? Maybe with deprecation warning? I suppose this a bigger-picture question of how all the updated functions should work in v0.7.0. If it requires a yasa.Hypnogram instance, then maybe all examples should just use the method rather than taking the Hypnogram as input?

After addressing this, I'll submit the PR after #121 is merged into master to avoid potential conflicts.

raphaelvallat commented

Awesome looking forward to it! I'll finish reviewing the other PR today or tomorrow.

So, for plot_hypnogram, I would just go full-on with the new Hypnogram class. This will greatly simplify the code, and this function was introduced recently so I think it's okay to make API-breaking changes. In other words, using an array_like input is not allowed anymore.

For newly-introduced Hypnogram methods (sleep_statistics, transition_matrix, find_periods)

  • Keep original implementation for now, add a DeprecationWarning, and in the future (v0.8?) remove them from the public API

For yasa.SleepStaging

  • Add FutureWarning saying that in the future (v0.8.0) YASA will return a yasa.Hypnogram object. Or should we make the switch already in YASA v0.7.0?

For all other functions (events detection, spectral)

  • It's tricky because these functions expect the upsampled integers hypnogram, as now returned from yasa.Hypnogram.upsample_to_data. Furthermore, these functions are the oldest and probably the most widely-used so we need to be very gradual. I'm not even sure that adding a Deprecation or FutureWarning is needed for v0.7.

Curious to hear your thoughts on the above.

raphaelvallat commented

For plot_hypnogram, you can also do what I did for sleep_statistics, i.e. duplicate the code for the Hypnogram method and keep the original implementation intact. If doing that, then we should add a DeprecationWarning to the original implementation, and in the future remove it from the public API, as with all other hypnogram-related functions. Your preference will be mine!

remrama commented

Awesome, this all sounds good. I was hoping we could ditch the array input! 👍

  • Yes, v0.8 sounds like a good removal target for the other utility functions.
  • I would suggest going ahead with returning a Hypnogram object from yasa.SleepStaging in v0.7. I'm thinking the new object-oriented structure might be too subtle of a change if not being returned by .predict(), since that is the primary way people are dealing with hypnograms in YASA. So it seems like all the code examples and tutorial would need an additional line that converts the returned array to a Hypnogram instance. Or maybe you like that explicit-ness about it? I'm okay with whatever but my intuition is to go full switch.
  • As for the "other" functions (eg, detection), I don't have strong opinions here. think it would be reasonable to return a Hypnogram from SleepStaging and then feed the Hypnogram.upsample_to_data() output to them, with a FutureWarning that they might just take the Hypnogram itself later. Or I could see them not being changed much at all (as I think you're suggesting). If SleepStaging still returned an array, then they might not even notice the change?

For plot_hypnogram, you can also [...] duplicate the code for the Hypnogram method and keep the original implementation intact.

I've already taken the approach of changing the original implementation, but lmk if you want me to switch back. Currently, the original function takes a Hypnogram object as input, and then the method is just a one line of return plot_hypnogram(self, **kwargs). I prefer this because it keeps the plotting libraries isolated and also avoids being pretty invasive on the yasa.Hypnogram source code. I think a full transfer from function to method would disproportionately invade that file, especially if other plotting variants/options get added in the future.

But one thing about this: I have a few examples that looks like plot_hypnogram(hyp, ...) and others like hyp.plot_hypnogam(...). Both work for now, but I'm wondering if I should only show examples that use the Hypnogram method to avoid confusion?

raphaelvallat commented

I would suggest going ahead with returning a Hypnogram object from yasa.SleepStaging in v0.7

You're right — let's do it!

Regarding the other functions (detections), I think for now let's not change the input. We will require the users to manually use the Hypnogram.upsample_to_data method, which is close to what they're already doing anyway. I have been considering that maybe we can take care of the upsampling internally such that the users would just pass spindles_detect(hypno=<yasa.Hypnogram at 30-seconds> , include=["N2", "N3"]). But at the same time I kinda like that the users are responsible for the upsampling.

👍 on your approach for plot_hypnogram.

only show examples that use the Hypnogram method to avoid confusion?

Yup!

  • 👍1
remrama commented

But at the same time I kinda like that the users are responsible for the upsampling.

Ha yes I hadn't thought about this... that the power of the Hypnogram object is so strong that it might risk a dangerous disconnect between the user and the underlying analysis! Good to be aware of and I like your idea of keeping some steps in.

  • 👍1
raphaelvallat commented

the power of the Hypnogram object is so strong

This will be the first line in the new release changelog 😂

  • 😄1