MRG, ENH: Improve Epochs._name and Evoked.comment by hoechenberger · Pull Request #9027 · mne-tools/mne-python

hoechenberger · 2021-03-11T15:01:02Z

Rework Epochs._name and Evoked.comment:

* replaced with × in Epochs._name
- old: 0.50 * auditory/left + 0.50 * visual/left
- new: 0.50 × auditory/left + 0.50 × visual/left
Evoked.comment in evoked differences created via mne.combine_evoked() now groups conditions via parens:
- old: 1.500 × 0.50 * auditory/left + 0.50 * visual/left + -1.500 × × 0.51 * auditory/right + 0.49 * visual/right
- new: 1.500 × (0.50 × auditory/left + 0.50 × visual/left) - 1.500 × (0.51 × auditory/right + 0.49 × visual/right)
special-casing for 1 and -1:
- old: -1.000 × 0.50 * auditory/left + 0.50 * visual/left + 1.000 × 0.51 * auditory/right + 0.49 * visual/right
- new: - (0.50 × auditory/left + 0.50 × visual/left) + (0.51 × auditory/right + 0.49 × visual/right)

Example using mne.viz.plot_compare_evokeds():

old:

new:

What do you think?

(btw unfortunately Evoked.comment cannot contain a proper minus character, as saving will fail then)

drammock · 2021-03-11T16:00:50Z

(btw unfortunately Evoked.comment cannot contain a proper minus character, as saving will fail then)

I'm surprised that it allows × but not − ... I'm guessing the FIF format encodes text in something bigger than ASCII but not as big as UTF-8?

BTW, this is still marked as draft. LMK when it's ready for review.

hoechenberger · 2021-03-11T16:33:24Z

I'm surprised that it allows × but not −

I could imagine that × gets mapped to ASCII 158?

BTW, this is still marked as draft. LMK when it's ready for review.

Just wanted to hear your general feeling about this before proceeding & fixing all tests etc :) So … no strong objections so far?

– Oh well, you know what, I'm just gonna mark this as ready for review :)

agramfort

very useful @hoechenberger ! you have my full support !

thanks for tackling this

…ith weight=-1

drammock · 2021-03-11T22:45:34Z

@hoechenberger is it ready for review now? Several more commits were just pushed while I was mid-review.

hoechenberger · 2021-03-11T22:49:30Z

e9dc642 fixes a bug that is present in main, related to the string representation / .comment of -Evoked:

main:
Evoked.comment: 0.50 * auditory/left + 0.50 * visual/left
-Evoked.comment: -0.50 * auditory/left + 0.50 * visual/left (minus sign only negates the first term)

with e9dc642

Evoked.comment: 0.50 × auditory/left + 0.50 × visual/left (unchanged except for multiplication signs)
-Evoked.comment: - (0.50 × auditory/left + 0.50 × visual/left) (minus sign correctly applied to all terms)

hoechenberger · 2021-03-11T22:53:38Z

@hoechenberger is it ready for review now? Several more commits were just pushed while I was mid-review.

Oh noes, I'm sorry about this! I rebased and added a few commits … It is ready for review now. Ouch … I owe you a review, I guess 😱

hoechenberger · 2021-03-11T22:56:10Z

We're using proper minus signs now in the plot legend for plot_compare_evokeds() (replacing the ASCII hyphens from Evoked.comment with Unicode minuses)

mne/evoked.py

drammock · 2021-03-11T23:41:41Z

Ouch … I owe you a review, I guess

Don't sweat it :) But it's the end of my day now, so I'll have to wait until tomorrow morning

mne/evoked.py

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

…oked-comment

…python into epochs-evoked-comment

larsoner

Probably needs latest.inc in the BUG section since this is technically a backward-incompatible change (some names in files / instances and accessibility via read_evokeds will change, but for the better / as a bugfix)

hoechenberger · 2021-03-22T19:47:04Z

@larsoner I've added a changelog entry to the Enhancements section

larsoner · 2021-03-22T19:49:49Z

The choice of Bugs was intentional, that is where we list backward-incompatible / breaking code changes without a deprecation period. So there must be an entry there. You could also have one in enhancements, but I don't see the point once you put one in Bugs...

drammock

Other than @larsoner's request to move the changelog entry, I have just one nitpick, not a blocker.

mne/viz/evoked.py

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

hoechenberger · 2021-03-22T20:03:39Z

The choice of Bugs was intentional, that is where we list backward-incompatible / breaking code changes without a deprecation period. So there must be an entry there. You could also have one in enhancements, but I don't see the point once you put one in Bugs...

I do see what you mean, and I agree that Enhancements might not be the right section. But the only reason to put it in Bugs, IMHO, is the historical argument you brought up – "we've always put it there in the past".

Can we not introduce a new section, Behavior changes, for stuff like that? or rename API changes to API and behavior changes? We've done exactly this in MNE-BIDS

…oked-comment

larsoner · 2021-03-22T20:15:27Z

But the only reason to put it in Bugs, IMHO, is the historical argument you brought up – "we've always put it there in the past".

I don't think of it as historical but rather as functional based on our current working definitions for each section: "Bugs" is where people should currently look for backward-incompatible changes that might break their code without any warning.

Can we not introduce a new section, Behavior changes, for stuff like that?

My response is the same as last time renaming changelog sections came up in a PR -- let's discuss in a dedicated issue/PR dedicated to that...

hoechenberger · 2021-03-22T20:31:07Z

My response is the same as last time renaming changelog sections came up in a PR -- let's discuss in a dedicated issue/PR dedicated to that...

Believe it or not, but I had forgotten about the fact that we'd been there before … ouch.

Alright then :)

hoechenberger · 2021-03-22T22:13:02Z

This should be good to merge now.

drammock · 2021-03-22T22:55:46Z

thx @hoechenberger !

hoechenberger changed the title ~~ENH: Improve Epochs._name and and Evoked.comment~~ ENH: Improve Epochs._name and Evoked.comment Mar 11, 2021

hoechenberger requested review from agramfort and drammock and removed request for agramfort March 11, 2021 15:16

hoechenberger changed the title ~~ENH: Improve Epochs._name and Evoked.comment~~ WIP, ENH: Improve Epochs._name and Evoked.comment Mar 11, 2021

hoechenberger marked this pull request as ready for review March 11, 2021 16:33

agramfort reviewed Mar 11, 2021

View reviewed changes

hoechenberger added 4 commits March 11, 2021 23:42

ENH: Improve Epochs._name and and Evoked.comment

50a6274

Special handling for minus sign in first term & during plotting

9c9cd21

Move prettyfication to aux function

48ceba1

Fix bug in string repr for -evoked; fix tests; special-case -evoked w…

e9dc642

…ith weight=-1

hoechenberger force-pushed the epochs-evoked-comment branch from b9f64a7 to e9dc642 Compare March 11, 2021 22:43

Style

52ce994

hoechenberger commented Mar 11, 2021

View reviewed changes

mne/evoked.py Outdated Show resolved Hide resolved

hoechenberger commented Mar 11, 2021

View reviewed changes

mne/evoked.py Outdated Show resolved Hide resolved

hoechenberger commented Mar 11, 2021

View reviewed changes

mne/evoked.py Outdated Show resolved Hide resolved

hoechenberger changed the title ~~WIP, ENH: Improve Epochs._name and Evoked.comment~~ ENH: Improve Epochs._name and Evoked.comment Mar 12, 2021

hoechenberger commented Mar 12, 2021

View reviewed changes

mne/evoked.py Outdated Show resolved Hide resolved

Update comments [skip azp][skip github]

ab26ebe

drammock reviewed Mar 12, 2021

View reviewed changes

mne/evoked.py Outdated Show resolved Hide resolved

mne/evoked.py Outdated Show resolved Hide resolved

Apply suggestion by @drammock

d11daaa

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

hoechenberger changed the title ~~ENH: Improve Epochs._name and Evoked.comment~~ MRG, ENH: Improve Epochs._name and Evoked.comment Mar 14, 2021

hoechenberger and others added 3 commits March 22, 2021 19:07

Update mne/evoked.py

f525eca

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

Merge branch 'main' of github.com:mne-tools/mne-python into epochs-ev…

de71aa4

…oked-comment

Merge branch 'epochs-evoked-comment' of github.com:hoechenberger/mne-…

1e139a2

…python into epochs-evoked-comment

hoechenberger requested a review from drammock March 22, 2021 18:09

larsoner approved these changes Mar 22, 2021

View reviewed changes

Add changelog entry [skip gh][skip github][circle full]

cf36b99

drammock approved these changes Mar 22, 2021

View reviewed changes

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

Update mne/viz/evoked.py

0e1fc07

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

hoechenberger added 2 commits March 22, 2021 21:08

Move changelog entry to Bugs section

dd846f3

Merge branch 'main' of github.com:mne-tools/mne-python into epochs-ev…

ff7552a

…oked-comment

get rid of :attr:

710e47d

drammock merged commit c2d4e4b into mne-tools:main Mar 22, 2021

hoechenberger deleted the epochs-evoked-comment branch March 23, 2021 07:18

larsoner mentioned this pull request Apr 12, 2021

MRG: Return empty list of SSP projectors if no ECG, EOG events were found #9277

Merged

Uh oh!

Conversation

hoechenberger commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drammock commented Mar 11, 2021

Uh oh!

hoechenberger commented Mar 11, 2021

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

drammock commented Mar 11, 2021

Uh oh!

hoechenberger commented Mar 11, 2021

Uh oh!

hoechenberger commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoechenberger commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drammock commented Mar 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

hoechenberger commented Mar 22, 2021

Uh oh!

larsoner commented Mar 22, 2021

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoechenberger commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Mar 22, 2021

Uh oh!

hoechenberger commented Mar 22, 2021

Uh oh!

hoechenberger commented Mar 22, 2021

Uh oh!

drammock commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hoechenberger commented Mar 11, 2021 •

edited

Loading

hoechenberger commented Mar 11, 2021 •

edited

Loading

hoechenberger commented Mar 11, 2021 •

edited

Loading

hoechenberger commented Mar 22, 2021 •

edited

Loading