remrama commented 2 years ago

Makes a minor adjustment to the underlying yasa.plot_hypnogram code. The hypnogram line is now drawn using a combination of plt.stairs and plt.hlines instead of plt.step. See Issue #106 for motivation behind this change.

Also adds a fill_color argument yasa.plot_hypnogram for an optional solid-color fill above the hypnogram line. Default value (None) will not draw filling.

remrama commented 2 years ago

Note that I only used ax.hlines for the REM and Art/Uns highlighting because of the poor handling of masked arrays in ax.stairs.

raphaelvallat commented 2 years ago

@remrama you're amazing!!

[x] Can you please also copy-paste these changes to the yasa.plot_spectrogram function (I'd say without the fill_color argument)? https://github.com/raphaelvallat/yasa/blob/399d98472c5f75c9dbba497a9e6012ae538ae1a4/yasa/plotting.py#L279-L281
[x] Can you please copy-paste here the generated docstring plots?

raphaelvallat commented 2 years ago

And edit the changelog please! Version 0.6.3.dev

remrama commented 2 years ago

No problem!

wrt the yasa.plot_spectrogram code, are you cool with instead replacing the fig argument with ax in yasa.plot_hypnogram and just using yasa.plot_hypnogram(..., ax=ax0) under the hood of yasa.plot_spectrogram? I've glanced at it and I don't foresee problems. Of course this avoids redundancies and is in general more flexible.

raphaelvallat commented 2 years ago

Agreed, good idea!

codecov-commenter commented 2 years ago

Codecov Report

Base: 91.41% // Head: 91.43% // Increases project coverage by +0.02% :tada:

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

:exclamation: Current head bf89ed8 differs from pull request most recent head 7a10187. Consider uploading reports for the commit 7a10187 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #108 +/- ## ========================================== + Coverage 91.41% 91.43% +0.02% ========================================== Files 22 22 Lines 2690 2674 -16 ========================================== - Hits 2459 2445 -14 + Misses 231 229 -2 ``` | [Impacted Files](https://codecov.io/gh/raphaelvallat/yasa/pull/108?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Raphael+Vallat) | Coverage Δ | | |---|---|---| | [yasa/plotting.py](https://codecov.io/gh/raphaelvallat/yasa/pull/108/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Raphael+Vallat#diff-eWFzYS9wbG90dGluZy5weQ==) | `98.69% <100.00%> (+1.59%)` | :arrow_up: | | [yasa/tests/test\_plotting.py](https://codecov.io/gh/raphaelvallat/yasa/pull/108/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Raphael+Vallat#diff-eWFzYS90ZXN0cy90ZXN0X3Bsb3R0aW5nLnB5) | `100.00% <100.00%> (ø)` | | | [yasa/tests/test\_spectral.py](https://codecov.io/gh/raphaelvallat/yasa/pull/108/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Raphael+Vallat#diff-eWFzYS90ZXN0cy90ZXN0X3NwZWN0cmFsLnB5) | `100.00% <100.00%> (ø)` | | | [yasa/detection.py](https://codecov.io/gh/raphaelvallat/yasa/pull/108/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Raphael+Vallat#diff-eWFzYS9kZXRlY3Rpb24ucHk=) | `97.73% <0.00%> (-0.12%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Raphael+Vallat). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Raphael+Vallat)

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

remrama commented 2 years ago

Should be good now @raphaelvallat! Attaching a figure that should show up during docs, or did you want the actual docs-generated one?

A few notes:

Good thing you asked for an update on the spectrogram plot. I ran into an issue plotting with ax.stairs because the sampling frequency is so much higher in that example! Previously, I was drawing a separate line for every epoch -- even if they were the same for a bunch of continuous epochs. That is very slow with a high sampling frequency. So I added a small section to yasa.plot_hypnogram that now finds all the continuous epochs and just draws one line per every break-point in the hypnogram. It's a good catch.
I hope it's alright I consolidated the code of yasa.plot_spectrogram a bit. Now that you don't have to do all the hypnogram work, it's a pretty straight-forward function. Actually, I think in the future you might drop the hypno option and just have an example in the docs where you open a figure with 2 axes and then yasa.plot_spectrogram(ax=ax1) and yasa.plot_hypnogram(ax=ax0). Not a big deal so just leaving it for now.
I saw there were no unittests for yasa.plot_spectrogram. Maybe there's a reason for that so I left it alone, but lmk if you want me to add some.

Figure_1

raphaelvallat commented 2 years ago

Awesome, looks great! I'll do an in-depth review of the code soon. FYI there are unit tests for the plot_spectrogram function:

https://github.com/raphaelvallat/yasa/blob/399d98472c5f75c9dbba497a9e6012ae538ae1a4/yasa/tests/test_spectral.py#L152-L169

raphaelvallat commented 2 years ago

Hi @remrama,

Quick question: When using fill_color with an hypnogram that contains ART and UNS, wouldn't it be better not to fill the ART and UNS? Currently the output looks like:

import numpy
import yasa
import matplotlib.pyplot as plt
hypno = 100 * [-2] + list(np.repeat([-0, 0, -1, -1, 0, 0, 1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 0, 0, 0], 120)) + 100 * [-2]
_, ax = plt.subplots(1, 1, figsize=(9, 4))
yasa.plot_hypnogram(hypno, sf_hypno=1/10, lw=1.5, fill_color="gainsboro");

That said I don't have a strong preference here.

raphaelvallat commented 2 years ago

Also, do you have a preference for using

plt.figure(figsize=(7, 3), constrained_layout=True)
ax = yasa.plot_hypnogram(hypno, fill_color="gainsboro")

or

fig, ax = plt.subplots(1, 1, figsize=(7, 3))
yasa.plot_hypnogram(hypno, fill_color="gainsboro", ax=ax)

remrama commented 2 years ago

Good call on the Art/Uns. There was a simple fix, clipping the hypno to only wake+ epochs when passing to the ax.stairs used for filling. Resulting pic attached.

wrt returning ax or not, no I don't have a preference. I've switched it up to return and pass ax through. Though I think I do have a small preference for leaving constrained_layout or adding plt.tight_layout() as a final line. Without one of these, the x-axis label is off the figure, and I think it's better for users to see an example that is pretty without needing adjustments. I left it there but would still change it if you'd like.

Figure_1

raphaelvallat commented 1 year ago

Perfect! Almost there, I just added one last comment re: order of the code. Thanks again!

remrama commented 1 year ago

K pushed up that change.

btw, I didn't do any rebasing/merging with master (1 commit behind). I figure you can handle that in this instance, but for future reference, do you have a preference on whether I should rebase vs merge master into whatever my feature-branch is before submitting a PR? Not sure how you like the commit history to flow. Or is this highly situation-dependent and I should figure it out myself...

raphaelvallat commented 1 year ago

Rebase whenever possible. Thanks so much again, merging now!

raphaelvallat / yasa

Stairs plot #108

Codecov Report