MRG, BUG: Fix combine evokeds

[drammock]

closes #7865
closes #7874

still WIP because I need to check the examples/tutorials to make sure they're all consistent with the docstring. Also needs what's new.

[agramfort]

Suggested change

	# ERPs.
	# of ERPs.

[hoechenberger]

I would say that "difference ERPs" is a valid term on its own, so no need to say "difference of ERPs" :)

[agramfort]

Suggested change

	weights = np.squeeze(weights).astype(float)
	weights = np.array(weights).astype(float)

I hate squeeze magic

[hoechenberger]

I don't see why we would need to use squeeze here anyway…?

[drammock]

It's not really necessary. My thinking was that we are testing that ndim == 1, and I don't think the docstring explicitly says that weights must be a 1D array, so I thought using squeeze was being generous to the user. I don't mind changing it back though

[agramfort]

fair enough. I am not enough an ERP person ;)

…

[drammock]

I've examined all of the examples/tutorials that use combine_evoked and it appears that all the figures match up exactly between master and this PR (other than the plot_eeg_erp tutorial which I've just corrected). So should be ready for merge when CIs are happy.

[agramfort]

Thx @drammock

[agramfort]

@drammock shall we backport this? at least the fix in tutorial?

[drammock]

I've been wondering about backporting. The behavior that @SophieHerbst reported was a bug, which is a good reason to backport. On the other hand, this PR also changes the behavior and guidance around how to do evoked subtraction. I'd rather introduce a change like that with a regular release, when people are more likely to read the changelog. I think the latter consideration is stronger for me. @larsoner what is your opinion?

[hoechenberger]

@drammock
I ran into something in our docs that is contradictory and therefore confusing:

Keeping track of nave is important for inverse imaging, because it is used to scale the noise covariance estimate (which in turn affects the magnitude of estimated source activity). See The minimum-norm current estimates for more information (especially the Whitening and scaling section). For this reason, combining Evoked objects with either weights='equal' or by providing custom numeric weights should usually not be done if you intend to perform inverse imaging on the resulting Evoked object.

(from https://mne.tools/dev/auto_tutorials/evoked/plot_10_evoked_overview.html#combining-evoked-objects, highlight by me)

vs

contrast = combine_evoked(evoked, weights=[-1, 1])  # Faces - scrambled
...
inverse_operator = make_inverse_operator(contrast.info, forward, noise_cov,
                                         loose=0.2, depth=0.8)

# Compute inverse solution on contrast
stc = apply_inverse(contrast, inverse_operator, lambda2, method, pick_ori=None)

(from https://mne.tools/dev/auto_examples/datasets/spm_faces_dataset.html#from-raw-data-to-dspm-on-spm-faces-dataset)

As a user, what shall I do? 😭😭😭

[larsoner]

I think the latter consideration is stronger for me. @larsoner what is your opinion?

I'm okay with not backporting

Keeping track of nave is important for inverse imaging, because it is used to scale the noise covariance estimate (which in turn affects the magnitude of estimated source activity). See The minimum-norm current estimates for more information (especially the Whitening and scaling section). For this reason, combining Evoked objects with either weights='equal' or by providing custom numeric weights should usually not be done if you intend to perform inverse imaging on the resulting Evoked object.

I'm pretty sure nave only has an effect for dSPM and sLORETA, and our weights math should be correct so actually we should still get this scale factor correct. So maybe we should get rid of this, unless @agramfort sees places it might be problematic that I'm missing.

[agramfort]

I agree with @larsoner. I am also not so sure it's problematic to use equal or custom weights as the nave of the output should still be correct. But maybe it requires empirical evidence.

[drammock]

actually I guess you're right... it's only grand_average where that warning is relevant (it sets nave=len(evokeds)). I'll change the tutorial text today.

[drammock]

crossref #7881