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

TRCA variation #39

Merged
merged 15 commits into from
Apr 26, 2021
Merged

TRCA variation #39

merged 15 commits into from
Apr 26, 2021

Conversation

ludovicdmt
Copy link
Contributor

Hello,

In addition to the original method of TRCA, I propose in this PR a variation that use regularization and riemannian mean for the computation of the matrix S (instead of euclidean mean as in original implementation). It provides improvements when data are noisy and have access to large number of calibration data.

This variation is inspired by a similar work from A. Barachant on CSP: https://hal.archives-ouvertes.fr/hal-00602686/document

Ideas are :

  • Empirical estimator for covariance matrix used in original paper is unbiased but with high variance especially with a high number of channel. It could benefit of some regularization.
  • The matrix S in TRCA method represent the average (euclidean) inter-trial covariance matrix. Covariance matrices are semi-definite positive and lie on a riemannian manifold. A measure of distance (geodesic distance) is defined on this curvated manifold: the riemannian mean. Euclidean mean does not respect the curvature of the space.
    However the quality of the estimation of the riemannian mean depends on the number of covariance matrices used.

It does not provide an improvement of performance for the specific dataset used in ./testd/data/trcadata.mat but we notice at least 10% of improvement on our data that are more noisy and use more calibration data (about 10 trial per class).

This variation rely on Pyriemann toolbox which is already in requirements.

Regularization in covariance matrices estimations + riemannian mean instead of euclid mean for S computation
@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #39 (40735c4) into master (4aa4ba4) will increase coverage by 0.33%.
The diff coverage is 95.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   77.94%   78.27%   +0.33%     
==========================================
  Files          20       20              
  Lines        2158     2200      +42     
==========================================
+ Hits         1682     1722      +40     
- Misses        476      478       +2     
Impacted Files Coverage Δ
meegkit/utils/trca.py 93.75% <93.33%> (+2.57%) ⬆️
meegkit/trca.py 95.87% <96.15%> (-1.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4aa4ba4...40735c4. Read the comment docs.

Copy link
Owner

@nbara nbara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ludovicdmt, that's great thanks a lot! I've left a few comments if you could please address them?

If you don't have time I'll do it myself later this week. I'll also try to improve the example_trca.py a bit

meegkit/trca.py Outdated Show resolved Hide resolved
meegkit/trca.py Outdated Show resolved Hide resolved
meegkit/utils/trca.py Outdated Show resolved Hide resolved
meegkit/utils/trca.py Outdated Show resolved Hide resolved
meegkit/utils/trca.py Show resolved Hide resolved
meegkit/utils/trca.py Outdated Show resolved Hide resolved
@nbara nbara added the enhancement New feature or request label Apr 20, 2021
@ludovicdmt
Copy link
Contributor Author

Thanks for the feedback, I'll have a look and do that tomorrow.

@ludovicdmt
Copy link
Contributor Author

Should be good now

@nbara
Copy link
Owner

nbara commented Apr 22, 2021

I'm a bit concerned by the very poor performance compared to the original method.

A performance drop can be expected, but in this cas we sometimes get ~30% accuracy on the example dataset.

The tests fail because I am testing all combinations of ensemble, method and regularization.

edit: Actually the accuracy drop is also visible even when always the euclidean mean (mean_covariance(S, metric='logeuclid')) with scm estimator, which in principle should be close to the original implementation.

edit2: actually metric='euclid with scm works ok-ish

meegkit/trca.py Show resolved Hide resolved
@ludovicdmt
Copy link
Contributor Author

Regarding performance, as explained in 1 geodesic mean is more robust to outliers than the euclidean mean. But it is also really sensible to ill-conditioned covariance matrices.
In this dataset, data are especially clean (an accuracy of 100% on some blocks). In addition, they are only 4 samples per class to estimate the $6\times6$ S matrix, so probably ill-conditioned estimations of covariance matrix S.

On data with smaller SNR and with more training samples, this TRCA variation gave us ~10% of performance improvement. I think the two methods are complementary in terms of use cases.

But maybe that's not the point of Meegkit toolbox which is more about collecting standard methods and I can't definitely understand that.

@nbara
Copy link
Owner

nbara commented Apr 22, 2021

The fact that the riemann method doesn't work as well on this dataset is a problem in itself, it's just that it makes it difficult to unit-test. For the original implementation, I can be reasonably sure that my code is correct because it yields good classification results. Here it's a bit more complex, and it will make catching potential future bugs (due to dependency changes, etc.) more difficult to track

@nbara
Copy link
Owner

nbara commented Apr 26, 2021

OK I restricted the test scope, and added an illustration to the example. Thanks @ludovicdmt

@nbara nbara merged commit e3e8cb9 into nbara:master Apr 26, 2021
@ludovicdmt
Copy link
Contributor Author

Ok thank you for the help and work @nbara !

@ludovicdmt ludovicdmt deleted the regul_TRCA branch April 26, 2021 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Information about filter bank analysis (FBA) and task-related component analysis (TRCA)
2 participants