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

RFC: Include the number of dropped epochs in the Epochs repr #12549

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

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Apr 17, 2024

For quick diagnostic purposes and sanity checks, I find it helpful being able to quickly see how many epochs were dropped. The changes in this PR include the number of dropped epochs in the string and HTML repr.

Before epochs were dropped:

<Epochs |  86 events (good & bad; none dropped), -0.1997950.499488 s, baseline -0.1997950 s, ~3.2 MB, data not loaded,
 '1': 20
 '2': 20
 '3': 20
 '4': 18
 '5': 4
 '32': 4>

After epochs were dropped:

<Epochs |  85 events (good & bad; 1 dropped), -0.1997950.499488 s, baseline -0.1997950 s, ~3.2 MB, data not loaded,
 '1': 20
 '2': 19
 '3': 20
 '4': 18
 '5': 4
 '32': 4>

After preloading:

<Epochs |  85 events (all good; 1 dropped), -0.1997950.499488 s, baseline -0.1997950 s, ~105.9 MB, data loaded,
 '1': 20
 '2': 19
 '3': 20
 '4': 18
 '5': 4
 '32': 4>

HTML rep:
Screenshot 2024-04-17 at 21 31 22

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')

# %%
epochs = mne.Epochs(raw, events=events)
print(repr(epochs))
epochs

# %%
epochs.drop(0)
print(repr(epochs))
epochs

# %%
epochs.load_data()
print(repr(epochs))
epochs

@hoechenberger hoechenberger marked this pull request as ready for review April 17, 2024 19:33
@hoechenberger hoechenberger changed the title Include the number of dropped epochs in the Epochs repr RFC: Include the number of dropped epochs in the Epochs repr Apr 17, 2024
@cbrnr
Copy link
Contributor

cbrnr commented Apr 17, 2024

In general, I'm not a huge fan of excessive reprs, but since the Epochs repr was already pretty long, it's probably fine to add this.

On a slightly related note, your example reminds me that the concepts of good, bad, and dropped epochs can be confusing, especially since these change after loading. I'm not sure including even more information in the repr helps to clarify the situation.

@mscheltienne
Copy link
Member

mscheltienne commented Apr 18, 2024

+1 for this change, but I agree with @cbrnr that the concepts of good/bad/dropped can be unclear. In your example, the good & bad and all good strings are confusing. Maybe good only would be a bit clearer after preload.

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 18, 2024

The notion of good and bad has been a source of confusion for all users of MNE I've ever encountered. I'd say it's out of scope for this PR to addresses this.

I'm inclined to revert my change to the string repr and only keep it for the HTML repr. would that be okay with you, @mscheltienne?

@mscheltienne
Copy link
Member

I would keep it for both repr, I think this change is beneficial in both cases.

@cbrnr
Copy link
Contributor

cbrnr commented Apr 18, 2024

Why do we need "good & bad" in the first place? Isn't this already reflected by "data not loaded"? I'd remove this and only show how many dropped epochs there are (if there aren't any, I would not include it). So:

<Epochs |  86 events, -0.199795 – 0.499488 s, baseline -0.199795 – 0 s, ~3.2 MB, data not loaded,
 '1': 20
 '2': 20
 '3': 20
 '4': 18
 '5': 4
 '32': 4>
<Epochs |  85 events (1 dropped), -0.199795 – 0.499488 s, baseline -0.199795 – 0 s, ~3.2 MB, data not loaded,
 '1': 20
 '2': 19
 '3': 20
 '4': 18
 '5': 4
 '32': 4>
<Epochs |  85 events (1 dropped), -0.199795 – 0.499488 s, baseline -0.199795 – 0 s, ~105.9 MB, data loaded,
 '1': 20
 '2': 19
 '3': 20
 '4': 18
 '5': 4
 '32': 4>

@hoechenberger
Copy link
Member Author

I like @cbrnr's suggestion

@cbrnr
Copy link
Contributor

cbrnr commented Apr 18, 2024

While you are at it, could you change the following minor things in the standard repr (to make them shorter):

  • There are two spaces after the |, there should be only one.
  • Times should be rounded to two decimal places.
  • There should be no spaces around .
  • Put parentheses around the baseline.

This would look like this:

<Epochs | 85 events (1 dropped), -0.20–0.50 s (baseline -0.20–0 s), ~105.9 MB, data loaded,
 '1': 20
 '2': 19
 '3': 20
 '4': 18
 '5': 4
 '32': 4>

Maybe we should even omit trailing zeros, i.e. -0.2–0.5 s (baseline -0.2–0 s).

@hoechenberger
Copy link
Member Author

There should be no spaces around –.

no, this will make a negative upper bound unreadable.

@hoechenberger
Copy link
Member Author

Maybe we should even omit trailing zeros, i.e. -0.2–0.5 s (baseline -0.2–0 s).

hard disagree, this inconsistency in precision hurts my eyes.

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 18, 2024

and really i don't want to touch any of this in this PR @cbrnr Can you maybe just open a new issue? Happy to discuss there

@cbrnr
Copy link
Contributor

cbrnr commented Apr 18, 2024

hard disagree, this inconsistency in precision hurts my eyes.

But it doesn't bother you in the current string? We show 0 and not 0.00 or even 0.000000...

@hoechenberger
Copy link
Member Author

hard disagree, this inconsistency in precision hurts my eyes.

But it doesn't bother you in the current string? We show 0 and not 0.00 or even 0.000000...

yep it does bother me actually 😅

@hoechenberger
Copy link
Member Author

hard disagree, this inconsistency in precision hurts my eyes.

But it doesn't bother you in the current string? We show 0 and not 0.00 or even 0.000000...

yep it does bother me actually 😅

then again I might be inclined to cut the 0 as a special case some slack 😅😅

@cbrnr
Copy link
Contributor

cbrnr commented Apr 18, 2024

See #12550 @hoechenberger.

@hoechenberger
Copy link
Member Author

Thanks @cbrnr!

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

3 participants