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

MRG: Revamp HTML reprs of Raw, Epochs, Evoked, and Info #12583

Merged
merged 111 commits into from
May 21, 2024

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Apr 27, 2024

For example, the Evoked repr contained the number of channels, but failed to subtract or report any bads.

Also I believe reporting the number of sampling points is quite pointless , so I removed that as well.

The table layout wasn't up-to-date either.

main (striped layout only added by VS Code, actually not present in the tables we produce!)

Screenshot 2024-04-28 at 00 36 50

This PR:

Screenshot 2024-04-28 at 00 41 39

For comparison, Epochs:

Screenshot 2024-04-28 at 00 36 15

MWE:

# %%
import mne

sample_dir = mne.datasets.sample.data_path()
sample_fname = sample_dir / "MEG" / "sample" / "sample_audvis_raw.fif"

raw = mne.io.read_raw_fif(sample_fname)
raw.crop(tmax=60)

events = mne.find_events(raw, stim_channel="STI 014")
raw.pick("eeg")

epochs = mne.Epochs(raw, events=events, preload=True)
epochs

# %%
evoked = epochs.average()
evoked

This one also
closes #12553

@agramfort
Copy link
Member

you're aligning to the minimum info by dropping number of channels and time points. I find it useful to know the duration and how many channels are available.

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 28, 2024

@agramfort Are you suggesting to add number of samples and channels to the Epochs repr instead, then? I could do that, no problem

@agramfort
Copy link
Member

agramfort commented Apr 28, 2024 via email

@hoechenberger
Copy link
Member Author

Ok i'll take a look!

@hoechenberger hoechenberger marked this pull request as draft April 28, 2024 12:04
@hoechenberger hoechenberger changed the title Update Evoked HTML repr to closer match the one from Epochs, both in design and in content Update HTMP reprs of Raw, Epochs, Evoked to be more consistent Apr 28, 2024
@hoechenberger
Copy link
Member Author

Ok, so I've now touched the Raw, Epochs, and Evoked HTML reprs to make things more consistent. Here's a comparison of what we have in main and what's in this PR branch now:

Comparison

@hoechenberger hoechenberger marked this pull request as ready for review April 28, 2024 17:37
@drammock
Copy link
Member

I'm generally quite happy with the improved consistency. Thanks! Nitpicks:

  • I find "digitized head and sensor positions" doesn't quite make sense (the headshape points aren't "head positions" in the sense that we normally use that term). I'd revert to "digitized points"
  • the "data kind" field is only in evoked; is that meant to communicate "avg" vs "stdev"? Does it make sense to collapse that into one field with nave (something like "average of NN epochs" or "standard deviation of NN epochs")?

@hoechenberger
Copy link
Member Author

I'm generally quite happy with the improved consistency. Thanks! Nitpicks:

  • I find "digitized head and sensor positions" doesn't quite make sense (the headshape points aren't "head positions" in the sense that we normally use that term). I'd revert to "digitized points"

I was worried this could be confused with "sampling points" on the time axis … I had first thought of labeling it "digitized had shape and sensor positions", but it's too long. I'm okay with changing it back to "digitized points", but it's not my favorite – maybe if we brainstorm a little, we can come up with a better idea?

  • the "data kind" field is only in evoked; is that meant to communicate "avg" vs "stdev"?

Yes, it was there before and I'm actually not sure if it's really needed?

Does it make sense to collapse that into one field with nave (something like "average of NN epochs" or "standard deviation of NN epochs")?

Good idea, I can do that!

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 28, 2024

@drammock Another thing we may need to address – these tables are quite long now, and I haven't added collapsible sections like we have for Raw (which essentially just prints out Info). Is that a problem for the tutorial rendering?

@hoechenberger hoechenberger changed the title Update HTMP reprs of Raw, Epochs, Evoked to be more consistent Update HTML reprs of Raw, Epochs, Evoked to be more consistent Apr 28, 2024
@mscheltienne
Copy link
Member

+1 for digitized points, unless someone comes up with a clearer alternative. Thanks for the upgrade, it looks great!

@hoechenberger
Copy link
Member Author

hoechenberger commented May 19, 2024

I think this is ready to be merged. It was quite challenging to build something that works and looks okay-ish in VS Code, Jupyter, and in Report. For example, I wanted to use the Popover API to create popups, but VS Code would render the Python Console input widgets above my popover content, while Jupyter wouldn't. Problems such as these.

Anyway, here are some examples:

VS Code Python Interactive, light theme

Screen.Recording.2024-05-19.at.09.14.44.mov

VS Code Python Interactive, dark theme

Screenshot 2024-05-19 at 09 15 33

Report, light theme

Screen.Recording.2024-05-19.at.09.57.58.mov

Report, dark theme

Screenshot 2024-05-19 at 09 58 33

Jupyter Lab, light theme

Screenshot 2024-05-19 at 10 00 49

Jupyter Lab, dark theme

Screenshot 2024-05-19 at 10 01 09

As you can see, there's a problem with dark theme support in Report and Jupyter Lab (but not in VS Code Python Interactive), as the collapse/uncollapse caret is simply changed to a white color, which of course only works if the background is dark.

I think for now this is good enough and I will try to improve dark theme support in some follow-up PRs; I've been wanting to add a dark theme to Report for a while now anyway.

Also the examples demonstrate that now, you can click on the entire section header row (not just on the carets) to collapse/uncollapse a section.

Rendered tutorial: https://output.circle-artifacts.com/output/job/d78b2fa9-df94-4d24-ad32-1b2806c11212/artifacts/0/html/auto_tutorials/intro/70_report.html#adding-raw-data

I snuck in a fix fox #12553 as well

@hoechenberger

This comment was marked as resolved.

@hoechenberger
Copy link
Member Author

I realized there's still some issues with the text alignment in Jupyter Lab, need to fix those first.

@hoechenberger
Copy link
Member Author

hoechenberger commented May 19, 2024

I think it's all fixed now in Jupyter Lab (light theme)!

Screenshot 2024-05-19 at 10 35 43

@hoechenberger
Copy link
Member Author

hoechenberger commented May 19, 2024

Ha! In my above "Jupyter Lab, dark theme" tests I only set my OS theme to dark, but not Jupyter. However, in CSS I'm of course querying the system. Setting Jupyter Lab to follow the system theme, I get the following in dark mode:

Screenshot 2024-05-19 at 21 03 00

which is just what we want.

So the only thing that's impaired for now in dark mode is Report, which I'll address sometime in the future.

@hoechenberger hoechenberger changed the title Revamp HTML reprs of Raw, Epochs, Evoked, and Info MRG: Revamp HTML reprs of Raw, Epochs, Evoked, and Info May 19, 2024
@larsoner
Copy link
Member

@mscheltienne feel free to review/merge if you're happy, or if you don't have time I can look

@mscheltienne
Copy link
Member

I'm away until Thursday, I can look then.

@hoechenberger
Copy link
Member Author

@larsoner If you want, go ahead and merge; and I can address comments from @mscheltienne in a follow-up PR (I expect we'll have to do a few of those once we start using the new code)

@drammock
Copy link
Member

let's get this in, I agree that follow-up PRs are fine

@drammock drammock merged commit c55c44a into mne-tools:main May 21, 2024
30 checks passed
@mscheltienne
Copy link
Member

Looks good to me, the only comment I have is on the pop-up you get when clicking on the number of channels (good or bad); on VSCode I don't find the notification very easy to use. But that's a very minor comment for a great refactor!

@hoechenberger hoechenberger deleted the evoked-html-repr branch May 24, 2024 10:42
hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request May 24, 2024
This aims to address a concern @mscheltienne voiced at
mne-tools#12583 (comment)

Now, the channel names can already be viewed by simply hovering
over the number of good or bad channels. The appearing overlay still lets the user know that by clicking, they can open the channel names in a popup window.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect rendering of embedded HTML title (?)
6 participants