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

ENH: refactor Xdawn and linear_regression_raw #2332

Open
jona-sassenhagen opened this issue Jul 23, 2015 · 6 comments
Open

ENH: refactor Xdawn and linear_regression_raw #2332

jona-sassenhagen opened this issue Jul 23, 2015 · 6 comments

Comments

@jona-sassenhagen
Copy link
Contributor

Xdawn and linear_regression_raw overlap a lot. Both use toeplitz to construct a big predictor matrix from event arrays, and return the coefficients of regressing the brain data. Possibly, linear_regression_raw could be extended slightly to function as a backend for Xdawn, as discussed with @alexandrebarachant .

The main issues:

  • Xdawn has an option to return the predictors, although it's not used within the function I think.
  • linear_regression_raw takes raw input, Xdawn epochs (although it seems the original intent was for Xdawn to take raw too) is probably the most complicated step.
  • Allowing Xdawn to call linear_regression_raw, and to read the result, should be straight-forward.

I don't think it would make much sense to refactor in the other direction (use Xdawn as the estimator for linear_regression_raw, or fully lose linear_regression_raw in favour of big Xdawn API enlargement) due to API and usage case differences

Maybe this should wait until #2331 has been addressed though.

@alexandrebarachant

@alexandrebarachant
Copy link
Contributor

yep, some overlap here.
I think the best is xdawn using your method, because it is more generic, and probably better.
The possibility of adding covariate is interesting, and could be exploited in the method.

Xdawn has an option to return the predictors, although it's not used within the function I think.

Xdawn use the predictor to recreate pseudo signal and estimates a covariance matrix used for the filters estimations. I think i can do without it but we have to check it does no impact performances significantly.

linear_regression_raw takes raw input, Xdawn epochs (although it seems the original intent was for Xdawn to take raw too) is probably the most complicated step.

Xdawn reconstruct signal from epochs. It is also why it raise an exception if you try to feed xdawn with overlapped epochs where the baseline has been corrected.
The quick way to use rERP would be to create a RawArray from the reconstructed signal and feed it to rERP

@kingjr
Copy link
Member

kingjr commented Aug 16, 2015

Small side points, that could be addressed during this refactoring:

  • I think Xdawn should use the sklearn convention i.e. fit and transform not fit and apply, WDYT? This is apparently already done, although I'm unclear on the difference between apply and transform..
  • It'd be nice to specify y manually.
  • @alexandrebarachant I'm reasking my question here, some other may be interested: would it be possible to change Xdawn to make it work with continuous regressors?

@alexandrebarachant
Copy link
Contributor

Hi,
There is already a transform that follow the sklearn api. apply works like the ICA, i.e. it apply filters and back-project the denoised signal in the sensor space. `transform' works like the CSP, i.e. it filter the signals.

The original intension of the code is to follow as close as possible the original algorithm. we can imagine a lot of variation, but then we should probably find a better name.

If we go back to the basic, Xdawn algorithm is in 3 step :

  • 1 Extract evoked responses (this is where it overlapps with rERP)
  • 2 Estimates the raw covariance matrix of the signal
  • 3 Get the spatial filters by joint eigenvalue decomp. of the covariance of the ERP (estimated by step 1) and the raw signal cov (from step 2)

Adding continuous regressor could be done in step 1, then the next part don't need any change.

@kingjr
Copy link
Member

kingjr commented Aug 16, 2015

Thanks for the clarifications!

The original intension of the code is to follow as close as possible the original algorithm. we can imagine a lot of variation, but then we should probably find a better name.

Ok, let's keep things separated then, and relabel if necessary.

Adding continuous regressor could be done in step 1, then the next part don't need any change.

Ok, I'd be happy to (/ need to) work a bit on this, but I'd need guidance. @jona-sassenhagen, where you thinking of something along these lines too?

@jona-sassenhagen
Copy link
Contributor Author

Yes.

@agramfort @alexandrebarachant thoughts on allowing XDawn to take Raw input?

@agramfort
Copy link
Member

Why do you want this ? If We do this we highly complexify the API

On 17 août 2015, at 13:37, jona-sassenhagen [email protected] wrote:

Yes.

@agramfort @alexandrebarachant thoughts on allowing XDawn to take Raw input?


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants