Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First implementation of the Hypnogram class #116

Merged
merged 40 commits into from
Dec 24, 2022
Merged

Conversation

raphaelvallat
Copy link
Owner

@raphaelvallat raphaelvallat commented Dec 16, 2022

Hi,

This PR introduces a first working implementation of the yasa.Hypnogram class, which will be the new standard way to deal with hypnograms in YASA moving forward, as discussed in #105.

This should greatly simplify with implementation of the performance evaluation pipeline (#78).

Remaining tasks

  • Unit tests for 3, 4 and 5-stages hypnogram (see coverage report)
  • Add Hypnogram.consolidate_stages method
  • Implement the Hypnogram.plot_hypnogram function. Here I'm not sure if we should re-implement from scratch, or instead use the existing yasa.plot_hypnogram function and add support to 2, 3 and 4-stages hypnograms. The latter will require adding the parameter n_stages to the function however. @remrama will implement in a separate PR.
  • Add compatibility with other YASA functions (e.g. detection). For now, users can simply use Hypnogram.as_int() to get a NumPy array with integer values.

@remrama I would appreciate your review on this (but no rush!). Let me know if you see any ideas for improvements and/or new class methods or properties that may be useful.

Thanks,
Raphael

@raphaelvallat raphaelvallat added the enhancement 🚧 New feature or request label Dec 16, 2022
@raphaelvallat raphaelvallat self-assigned this Dec 16, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2022

Codecov Report

Base: 91.64% // Head: 92.37% // Increases project coverage by +0.72% 🎉

Coverage data is based on head (b1cc5f9) compared to base (ca4a834).
Patch coverage: 99.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   91.64%   92.37%   +0.72%     
==========================================
  Files          22       23       +1     
  Lines        2753     3054     +301     
==========================================
+ Hits         2523     2821     +298     
- Misses        230      233       +3     
Impacted Files Coverage Δ
yasa/hypno.py 98.19% <98.94%> (+0.97%) ⬆️
yasa/tests/test_hypnoclass.py 100.00% <100.00%> (ø)
yasa/detection.py 97.73% <0.00%> (-0.12%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@remrama remrama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO COOL @raphaelvallat! No major suggestions from me. As usual, lots to say but not much of substance 🙃

yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
yasa/tests/test_hypnoclass.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@remrama remrama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current status approved in spirit, but I see you have more you'd like to implement so lmk when you want a full review with formal approval.

@remrama
Copy link
Collaborator

remrama commented Dec 23, 2022

Btw, you're call but I would support merging this as-is and then adding "full functionality" in a separate PR. I'm not sure how big of a project that is, but if just the base class was in master I'd start some PRs with it.

@remrama
Copy link
Collaborator

remrama commented Dec 23, 2022

Suggested method:

class Hypnogram:
    ...
    def summary(self): # or .get_dataframe
        """
        Return a pandas DataFrame summarizing epoch-level information.

        Column order and names are compliant with BIDS events files [BIDSevents]_
        and MNE events/annotations dataframes [MNEannotations]_.

        Returns
        -------
        summary : :py:class:`pandas.DataFrame`
            A dataframe containing epoch onset, duration, stage, etc.

        References
        ----------
        .. [BIDSevents] https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/05-task-events.html
        .. [MNEannotations] https://mne.tools/stable/glossary.html#term-annotations
        """
        data = {
            "onset": self.timedelta.total_seconds(),
            "duration": 1 / self.sampling_frequency,
            "value": self.as_int().to_numpy(),
            "description": self.hypno.to_numpy(),
            "epoch": 1 + np.arange(self.n_epochs),
        }
        if hypno.scorer is not None:
            data["scorer"] = hypno.scorer
        return pd.DataFrame(data)

@raphaelvallat
Copy link
Owner Author

@remrama agreed for the new method! What do you think about calling it Hypnogram.as_annotations() or Hypnogram.as_mne_annotations()?

Also, when start is not None, should onset be the actual datetime, or is it better to always have self.timedelta.total_seconds()? I'm guessing the latter is the standard MNE/BIDS format?

I'll wait for your reply, add this new method and then request your final approval before merging 🎉 !

@remrama
Copy link
Collaborator

remrama commented Dec 24, 2022

What do you think about calling it Hypnogram.as_annotations() or Hypnogram.as_mne_annotations()?

Either are cool with me, although it's probably more of a BIDS/events emphasis than an MNE/annotation emphasis.

Also, when start is not None, should onset be the actual datetime, or is it better to always have self.timedelta.total_seconds()? I'm guessing the latter is the standard MNE/BIDS format?

Ya good question. Definitely want to always have the onset column as seconds from start (ie, self.timedelta.total_seconds()), because this is the BIDS-standard which I think takes precedent. MNE is a bit more ambiguous (as far as I can tell), and sometimes returns onset as seconds or timestamps.

I think in this case, if the Hypnogram includes timestamp info it should be added as an additional column rather than replacing onset. It's not totally clear to me what this would be called, according to BIDS docs, this might be "sample" but I'm not sure. If unclear, I think we could just call it "timestamp".

@raphaelvallat
Copy link
Owner Author

In the future, maybe we could go even one step further and add an output_type="mne" parameter to this method, which could be either "mne" (returns a mne.Annotations object), or "dataframe" (default BIDS-like dataframe, which is also compatible with EDFBrowser).

Question: why start epoch at 1 and not 0?

{"epoch": 1 + np.arange(self.n_epochs)}

If unclear, I think we could just call it "timestamp".

After second thought, I think that for the initial implementation I would only include onset in seconds, and not the actual timestamps.

@raphaelvallat
Copy link
Owner Author

Maybe epoch could be set as the index of the resulting dataframe? Would that break things w.r.t to BIDS/MNE annotations format?

>>> from yasa import Hypnogram
>>> hyp = Hypnogram(["W", "W", "LIGHT", "LIGHT", "DEEP", "REM", "WAKE"], n_stages=4)
>>> hyp.as_annotations()
       onset  duration  value description
epoch                                    
1        0.0      30.0      0        WAKE
2       30.0      30.0      0        WAKE
3       60.0      30.0      2       LIGHT
4       90.0      30.0      2       LIGHT
5      120.0      30.0      3        DEEP
6      150.0      30.0      4         REM
7      180.0      30.0      0        WAKE

@remrama
Copy link
Collaborator

remrama commented Dec 24, 2022

Love the output_type idea!

Question: why start epoch at 1 and not 0?

Ya you called me out on this :) Start it at 0. I wasn't sure about that one, it's just that technically it is the 1st epoch, and the Python indexing can be confusing for new users. But you're right, submit to Python indexing.

I think that for the initial implementation I would only include onset in seconds, and not the actual timestamps.

Yep that's good. If someone really wants timestamp that can add it trivially with one more line.

Maybe epoch could be set as the index of the resulting dataframe?

That looks great! It won't break things, neither BIDS or MNE expect that column. It's not an incredibly useful column anyways, it's really just a row number... Leave it starting at 0 and so basically we're just renaming that axis to epoch.

@raphaelvallat
Copy link
Owner Author

Ready for your final review @remrama !

Copy link
Collaborator

@remrama remrama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it ✈️

yasa/hypno.py Outdated Show resolved Hide resolved
yasa/hypno.py Outdated Show resolved Hide resolved
raphaelvallat added a commit that referenced this pull request Dec 24, 2022
* Initial class creation for Hypnogram

* Add properties + mapping

* Add sleep_statistics and upsample

* Add mapping for all n_stages + SFI

* Fix in sleep_statistics when all WAKE

* Minor edits

* Added `scorer` and two new methods

* Add docstring

* Start adding unittest for Hypnogram class

* Add more docstring

* Add main docstring + unit test upsample_to_data

* Uncommented upsample function

* Add to API

* Add unit tests for 3-5 stages hypnogram

* Added consolidate_stages method

* First minor comments from PR review

* Initial class creation for Hypnogram

* Add properties + mapping

* Add sleep_statistics and upsample

* Add mapping for all n_stages + SFI

* Fix in sleep_statistics when all WAKE

* Minor edits

* Added `scorer` and two new methods

* Add docstring

* Start adding unittest for Hypnogram class

* Add more docstring

* Add main docstring + unit test upsample_to_data

* Uncommented upsample function

* Add to API

* Add unit tests for 3-5 stages hypnogram

* Added consolidate_stages method

* First minor comments from PR review

* Add docstring for method `upsample`

* Added as_annotations method

* Minor docstring edits

* Epoch starts at 0 in `as_annotations`

* Updated changelog

* Fix typo
@raphaelvallat raphaelvallat merged commit f55eb55 into master Dec 24, 2022
@raphaelvallat raphaelvallat deleted the hypnogram_class branch December 24, 2022 14:24
@raphaelvallat raphaelvallat linked an issue Dec 24, 2022 that may be closed by this pull request
@raphaelvallat
Copy link
Owner Author

Merged to master. Thanks so much for your help on this! The next release is going to be 🔥

@raphaelvallat raphaelvallat mentioned this pull request Jan 8, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🚧 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Hypnogram class
3 participants