Closed raphaelvallat closed 1 year ago
Base: 91.64% // Head: 92.37% // Increases project coverage by +0.72%
:tada:
Coverage data is based on head (
b1cc5f9
) compared to base (ca4a834
). Patch coverage: 99.33% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
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.
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)
@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 🎉 !
What do you think about calling it
Hypnogram.as_annotations()
orHypnogram.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, shouldonset
be the actual datetime, or is it better to always haveself.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".
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.
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
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
.
Ready for your final review @remrama !
Merged to master
. Thanks so much for your help on this! The next release is going to be 🔥
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
Hypnogram.consolidate_stages
methodHypnogram.plot_hypnogram
function. Here I'm not sure if we should re-implement from scratch, or instead use the existingyasa.plot_hypnogram
function and add support to 2, 3 and 4-stages hypnograms. The latter will require adding the parametern_stages
to the function however.~ @remrama will implement in a separate PR.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