ENH: make compute_covariance work with Evoked objects - #14129
ENH: make compute_covariance work with Evoked objects#14129BabaSanfour wants to merge 4 commits into
Conversation
|
@larsoner PR is ready; code works as intended, one improvement would be changing epochs to inst, this would require some deprecation cycles (?). |
| The epochs. | ||
| epochs : instance of Epochs | Evoked | list of Epochs | ||
| The epochs or evoked response. For an evoked response, time samples | ||
| are treated as observations. |
There was a problem hiding this comment.
if this can now also be Evoked, then the parameter should no longer be called epochs but rather inst (which is the name we use in other places when it can be several different things).
| "`keep_sample_mean=False` cannot be used when `epochs` is an " | ||
| "Evoked instance" | ||
| ) | ||
| return [epochs], True |
There was a problem hiding this comment.
I think it's better to let the function do one thing: unpack the covariance inputs, and not also return whether the instance was an Evokeds or not. It's easy enough to do with isinstance(inst, Evoked).
| epochs = _unpack_epochs(epochs) | ||
| else: | ||
| epochs = sum([_unpack_epochs(epoch) for epoch in epochs], []) | ||
| epochs, is_evoked = _unpack_covariance_inputs(epochs, keep_sample_mean) |
There was a problem hiding this comment.
also here it should be inst instead of epochs
|
This is useful, also for the upcoming dipole fitting GUI :) |
|
Thanks @wmvanvliet , i was going to make it an inst .. but not sure how i should handle deprecation here. Should i just change it ? |
|
it's only not compatible if someone explicitly does |
| epochs, | ||
| keep_sample_mean=True, | ||
| tmin=None, | ||
| tmax=None, | ||
| projs=None, | ||
| *, | ||
| on_few_samples="warn", |
There was a problem hiding this comment.
@BabaSanfour something like this:
| inst, | |
| keep_sample_mean=True, | |
| tmin=None, | |
| tmax=None, | |
| projs=None, | |
| *, | |
| epochs=None, # deprecated | |
| on_few_samples="warn", |
3 participants
Reference issue (if any)
Closes #14115.
What does this implement/fix?
Accept Evoked through existing epochs argument and reject keep_sample_mean=False. With Evoked data we treat samples will be treated as observations and channels as variables: spatial covariance over time, not an estimate of trial-to-trial noise covariance
Possible alternatives/Improvements
Additional information
With the help of claude Opus5; all lines added were read and double checked.