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

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

Open
wants to merge 96 commits into
base: main
Choose a base branch
from

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

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

@mscheltienne
Copy link
Member

It's actually a bit better 😆

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

it looks beautiful @hoechenberger 😍 And the refactoring of the Jinja templates resolves a big pain point --- it will be much easier to make other object types have consistent HTML reprs now too.

I left a couple minor inline comments/suggestions, here are three other tiny suggestions/ideas from looking at the rendered output:

  1. In the "general" section, "Data type" feels like not quite the right name (to me "data type" means float, int, complex, etc). Maybe "object type"?
  2. I'm very slightly bothered by the title of the "Data" section (seems too generic of a title). At first I thought "Times" (to parallel the "channels" section) would work instead --- for raw and epochs, the "data" section is all about time-domain things (duration, sfreq, events; events is not strictly about time, but if you squint it could make sense under a "times" heading). Sadly this doesn't work for evokeds, because there we also get "aggregation" and "condition" entries. I think it's OK to leave as "data" for now unless someone has a quick idea about a better alternative.
  3. I think it's confusing for "good channels" to say "59 EEG" (a count) and "bad channels" to say "EEG 053" (a channel name that looks a lot like a count of channels of a particular type). A couple possible repairs:
    a. channel names are always enquoted
    b. when there are few enough channels to be worth writing them individually, write a count followed by names in parentheses (e.g., bad channels: 1 (EEG 053)).

All of these are small enough that they can be done later / are not blockers for me

mne/epochs.py Outdated Show resolved Hide resolved
mne/evoked.py Outdated Show resolved Hide resolved

def _dt_to_str(dt: datetime.datetime) -> str:
"""Convert a datetime object to a human-reaable string representation."""
return dt.strftime("%B %d, %Y %H:%M:%S") + " UTC"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This could work, yes
I can try it out

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to work!

While we're at it: WDYT about a date representation like this:

2002-12-03 at 19:01:10 UTC

(YYYY-MM-DD)

Copy link
Member

Choose a reason for hiding this comment

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

YYYY-MM-DD

That is my personal preferred date format, so I'm fine with it :)
It's also what ISO 8601 stipulates, so it's as good a choice as any

Copy link
Member Author

Choose a reason for hiding this comment

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

@drammock I addressed this in fd976a5

I didn't implement "full" ISO-8601, because it's not super human-readable when you have a datetime string; but I took inspiration from that standard. Please LMK what you think

@hoechenberger
Copy link
Member Author

hoechenberger commented May 7, 2024

@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!)

Co-authored-by: Daniel McCloy <dan@mccloy.info>
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

thanks @hoechenberger !

@hoechenberger
Copy link
Member Author

@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
Copy link
Member Author

  • In the "general" section, "Data type" feels like not quite the right name (to me "data type" means float, int, complex, etc). Maybe "object type"?

I have addressed this in aacb2c4 and renamed it to "MNE object time", WDYT?

@hoechenberger
Copy link
Member Author

3. I think it's confusing for "good channels" to say "59 EEG" (a count) and "bad channels" to say "EEG 053" (a channel name that looks a lot like a count of channels of a particular type). A couple possible repairs:
a. channel names are always enquoted
b. when there are few enough channels to be worth writing them individually, write a count followed by names in parentheses (e.g., bad channels: 1 (EEG 053)).

I'll look into this. Thanks to the refactoring I've done here, I believe now I only have to touch the _format_channels() function (i.e., all is in one place). Neat!

@hoechenberger
Copy link
Member Author

hoechenberger commented May 8, 2024

@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

I renamed Data to Acquisition

@drammock
Copy link
Member

drammock commented May 8, 2024

I renamed Data to Acquisition

that's an improvement I think. The number averaged in an Evoked isn't really "acquisition" but I think it's OK.

@hoechenberger hoechenberger changed the title Update HTML reprs of Raw, Epochs, Evoked to be more consistent Revamp HTML reprs of Raw, Epochs, Evoked, and Info May 16, 2024
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.

None yet

6 participants