mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

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

Open hoechenberger opened this issue 1 year ago • 42 comments

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

hoechenberger avatar Apr 27 '24 22:04 hoechenberger

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.

agramfort avatar Apr 28 '24 10:04 agramfort

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

hoechenberger avatar Apr 28 '24 11:04 hoechenberger

yes I would have done that if you push for consistency.

Message ID: @.***>

agramfort avatar Apr 28 '24 11:04 agramfort

Ok i'll take a look!

hoechenberger avatar Apr 28 '24 11:04 hoechenberger

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 avatar Apr 28 '24 17:04 hoechenberger

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")?

drammock avatar Apr 28 '24 19:04 drammock

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 avatar Apr 28 '24 19:04 hoechenberger

@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 avatar Apr 28 '24 19:04 hoechenberger

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

mscheltienne avatar Apr 28 '24 21:04 mscheltienne

@mscheltienne I named the row now "Head shape and sensor digitization", and the value is then "xxx points" or "Not available"

WDYT?

hoechenberger avatar Apr 28 '24 21:04 hoechenberger

@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?

Ideally they'd have collapsible sections, yeah. Can you add them in this PR? If not please create a follow-up issue so someone can do it later.

drammock avatar Apr 28 '24 21:04 drammock

Ideally they'd have collapsible sections, yeah. Can you add them in this PR?

This then also raises the question of how much Info (pun intended) to include here

For Raw, the HTML repr currently is literally Raw.info with recording filename and duration added.

For Epochs and Evoked, it's a mixture of some instance attributes plus some select parts extracted from inst.info.

If we add collapsibles anyway, we could add the entire Info (since it's easy to hide additional information that way), but I'm sure @cbrnr would hate it :)

Re collapsibles, I like the idea per se, but the current implementation for Info is visually unpleasant, since each section gets its own table and hence the columns don't align. I wonder if we can find a better solution.

hoechenberger avatar Apr 29 '24 08:04 hoechenberger

Re collapsibles, I like the idea per se, but the current implementation for Info is visually unpleasant, since each section gets its own table and hence the columns don't align. I wonder if we can find a better solution.

I will look into this and propose a solution

hoechenberger avatar Apr 29 '24 08:04 hoechenberger

I don't like the idea to misuse standard reprs of Python objects. You are only modifying HTML reprs, which I think is fine (and I don't use them anyway, so I really have no opinion on what should and should not be part of these tables).

cbrnr avatar Apr 29 '24 09:04 cbrnr

Got you! @cbrnr

hoechenberger avatar Apr 29 '24 09:04 hoechenberger

Looks like there are plenty of folks chiming in here with ideas so as long as you all converge I don't think I need to look, just wanted to say thanks for looking into this!

larsoner avatar Apr 29 '24 13:04 larsoner

@larsoner If you have any thoughts on potentially a better label for "Digitized points", I'd love to hear it!

Maybe it's just me but in my ears it sounds kind of ambiguous / unspecific or even confusing when this information is presented in a summary table where we also display the number of time-series data sampling points

Any input welcome

hoechenberger avatar Apr 29 '24 13:04 hoechenberger

Re collapsibles, I like the idea per se, but the current implementation for Info is visually unpleasant, since each section gets its own table and hence the columns don't align. I wonder if we can find a better solution.

I will look into this and propose a solution

I've found a solution, but it may take a day or two to implement. Will report back once I've had the time to do that

hoechenberger avatar Apr 29 '24 14:04 hoechenberger

This is ready for review

I think a good way to explore these changes is by browsing through the Report tutorial, because the different examples there demonstrate the HTML repr for Raw, Epochs, and Evoked:

https://output.circle-artifacts.com/output/job/ce2dba84-e894-4f81-b168-a0b0af9807be/artifacts/0/html/auto_tutorials/intro/70_report.html#adding-raw-data

Note that for the documentation build, the repr sections are collapsed; during interactive use or when users create reports, all sections are expanded.

I consolidated repr sections across Info, Raw, Epochs, and Evoked.

Many helper functions were converted to Jinja2 filters, which allows us to do less "preparatory work" inside the _repr_html_() methods (e.g. for generating human-readable strings of datetimes); instead, this can now all be taken care of inside the Jinja2 templates, with only a few exceptions.

When displaying the recording time, I replaced the "GMT" timezone with "UTC", which I believe is more correct.

This PR contains one breaking API change: Info.ch_names used to raise an exception if Info["ch_names"] did not exist; now, it returns an empty list. This made certain operations for me easier. Since this should be a very rare edge case, I don't believe a deprecation cycle is necessary.

cc @agramfort

hoechenberger avatar May 03 '24 21:05 hoechenberger

honestly having to click on each section to see something is to me an overkill. It forces me to use my mouse and while before shift+enter is enough to see what I want. Could we find a middle ground here? eg you see the important info by default and you can click to have the full details? or expand certain sections by default?

nit: the + sign is not to me the most common. It's more classic to have > and that become / when it's expanded.

Message ID: @.***>

agramfort avatar May 04 '24 07:05 agramfort

@agramfort There might be a misunderstanding here, the clicking is only required in the documentation as is already the case with what we have in main

Edit: But you're right, I suppose even within the documentation the respective reprs should be expanded in the Report tutorial 🤔 But again, this problem is already present in main / stable

If instead you're using your Python Interactive window or creating a Report, everything is expanded by default, so no clicking is necessary!

Try this:

# %%
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).load_data()

events = mne.find_events(raw, stim_channel="STI 014")
raw.pick("eeg")
raw.set_eeg_reference(projection=True)  # to add a proj
raw

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

# %%
evoked = epochs.average()
evoked

# %%
raw.info

hoechenberger avatar May 04 '24 08:05 hoechenberger

the + sign is not to me the most common. It's more classic to have > and that become / when it's expanded.

Yep, I will try to find a replacement

hoechenberger avatar May 04 '24 08:05 hoechenberger

@agramfort

the + sign is not to me the most common. It's more classic to have > and that become / when it's expanded.

Yep, I will try to find a replacement

This has been resolved, see e.g.: https://output.circle-artifacts.com/output/job/9a976a2f-b617-4f47-9f43-8c4c1d998eae/artifacts/0/html/auto_tutorials/intro/70_report.html#adding-raw-data

hoechenberger avatar May 04 '24 14:05 hoechenberger

Latest rendered Report tutorial: https://output.circle-artifacts.com/output/job/6a2f0a7d-04bb-4581-8c01-0b2ef7bdbbbe/artifacts/0/html/auto_tutorials/intro/70_report.html#adding-raw-data

CI errors seem unrelated

hoechenberger avatar May 04 '24 15:05 hoechenberger

Dang, the carets (collapse / uncollapse buttons) don't show up on my iPad. Need to investigate.

hoechenberger avatar May 04 '24 16:05 hoechenberger

I think this is ready for review now. I addressed @agramfort's concerns regarding the collapse / uncollapse buttons (they are carets now instead of plus / minus symbols). It seems to look ok (not perfect) in Reports on both desktop and my iPad, and in the VS Code Python Interactive console.

The segments of the repr are all expanded by default, and only collapsed in the built documentation (as is the case with our stable docs).

Rendered Report tutorial: https://output.circle-artifacts.com/output/job/bb7905a0-af6b-4d52-a43e-b196baa58e63/artifacts/0/html/auto_tutorials/intro/70_report.html#adding-raw-data

We still need to make a decision regarding the naming of the digitization points field.

cc @mscheltienne

hoechenberger avatar May 07 '24 11:05 hoechenberger

Thanks for your feedback, @mscheltienne!

Looks great, the length of Head shape and sensor digitization is not bothering me.

I misread this as "is bothering me" and came up with a shorter proposal: "Head & Sensor Digitization" :) Thoughts?

hoechenberger avatar May 07 '24 13:05 hoechenberger

It's actually a bit better :laughing:

mscheltienne avatar May 07 '24 13:05 mscheltienne

@drammock I agree with all of these, including the naming of the "Data" section and the inconsistencies re the good / bad channels. However this is already present in main so I didn't really dare to touch it. But if we can come up with better ideas, I can certainly include them in this PR!

(Re Data: I actually spent a while thinking about it and exploring different names, but couldn't come up with anything better… happy to keep on brainstorming though!)

hoechenberger avatar May 07 '24 15:05 hoechenberger

@drammock FYI when I started with this work, I named the Data section Recording, which seemed nice until I realized for an Info object, it seemed kinda odd, because it doesn't have any "recorded" data attached.

That said, one could argue that a Data section is just as inappropriate there.

So what do you say, shall we rename Data -> Recording? I can push a commit so you can take a look at the rendered reprs to get a better idea of what it "feels" like

hoechenberger avatar May 08 '24 07:05 hoechenberger