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

Slightly improve Epochs repr #12550

Merged
merged 2 commits into from Apr 30, 2024
Merged

Slightly improve Epochs repr #12550

merged 2 commits into from Apr 30, 2024

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Apr 18, 2024

This PR slightly improves the Epochs repr from

<Epochs |  86 events (good & bad), -0.199795 – 0.499488 s, baseline -0.199795 – 0 s, ~3.2 MB, data not loaded,

to

<Epochs | 86 events (good & bad), -0.20 – 0.50 s (baseline -0.20 – 0.00 s), ~3.2 MB, data not loaded,

Changes include removing a space after the | symbol and rounding times to two decimal places.

I'm showing only the first line, since the other lines are not affected.

@hoechenberger
Copy link
Member

If we need to round, could we use 3 decimal places?

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 18, 2024

If we need to round, could we use 3 decimal places?

🤔 this will result in many trailing zeros for standard time segments. I'd prefer a dynamic approach that shows a maximum of 2 (or 3) decimal places. Would that be a big problem?

<Epochs | 86 events (good & bad), -0.2 – 0.5 s (baseline -0.2 – 0 s), ~3.2 MB, data not loaded,

If it bothers you that the four numbers show a different number of decimal places, we could also use the minimum number of decimal places for all numbers:

<Epochs | 86 events (good & bad), -0.2 – 0.5 s (baseline -0.2 – 0.0 s), ~3.2 MB, data not loaded,

But this would require a bit more complexity in the method.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 18, 2024

With three decimal places, it would look like this:

<Epochs | 86 events (good & bad), -0.2 – 0.499 s (baseline -0.2 – 0 s), ~3.2 MB, data not loaded,

@hoechenberger
Copy link
Member

I'm wondering if truncating instead of rounding might actually be the better option here? 🤔

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 19, 2024

I'm wondering if truncating instead of rounding might actually be the better option here? 🤔

Why? IMO rounding is the more accurate option than truncating...

@hoechenberger
Copy link
Member

I'm just thinking out loud, it's just a gut feeling ... truncating would make it more clear that / if the displayed value is not "exact", you know? If I see 0.199 instead of 0.2, I know that things might be off by one sample or so. Again, just brainstorming here..

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 19, 2024

Yes, it's always going to be a tradeoff, because we're showing time as opposed to samples. If you want accuracy, we should show samples. If we stick to time (which I'd prefer), we can just as well round to make the output a bit easier to read. Since we won't need more than 1ms of precision, I'd round to 3 digits as you suggested, and if the rounded value is e.g. -0.2 instead of -0.199795, then that's OK. I find that -0.199795 – 0.499488 s is really hard to read when all I want to know is the epoch definition to some precision. 6 digits is overkill for me. But yes, I get your point. What do others think?

@agramfort
Copy link
Member

showing rounding to ms seems ok to me

@drammock
Copy link
Member

I would vote for max of 3 digits (ms precision) but omit trailing zeros

@hoechenberger
Copy link
Member

I would vote for max of 3 digits (ms precision) but omit trailing zeros

+1, seems like a good solution to me after all

@hoechenberger
Copy link
Member

@drammock Feel free to merge if you're happy!

mne/epochs.py Show resolved Hide resolved
mne/epochs.py Show resolved Hide resolved
@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 30, 2024

@drammock anything else you want me to do? If not, please feel free to merge.

@hoechenberger
Copy link
Member

I'm going ahead with merging, since all comments seem to have been addressed.

@drammock if you disagree please ping me and i will revert

thanks @cbrnr

@hoechenberger hoechenberger merged commit dddbe78 into mne-tools:main Apr 30, 2024
30 checks passed
@cbrnr cbrnr deleted the epochs-repr branch April 30, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants