MRG, ENH: Improve Epochs._name and Evoked.comment

[hoechenberger]

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]

(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]

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

[drammock]

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

[hoechenberger]

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]

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

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)

[drammock]

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

[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]

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

[larsoner]

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.

[hoechenberger]

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

[larsoner]

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]

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]

This should be good to merge now.

[drammock]

thx @hoechenberger !