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

Add fixed point covariance estimator and add **kwds arguments in Covariances #220

Merged
merged 8 commits into from Jan 28, 2023

Conversation

qbarthelemy
Copy link
Member

@qbarthelemy qbarthelemy commented Jan 17, 2023

This PR:

  • adds a new covariance estimator: fixed point covariance matrix;
  • adds **kwds arguments in Covariances, for parameters passed directly to the covariance estimator.

Copy link
Member

@sylvchev sylvchev left a comment

Choose a reason for hiding this comment

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

Nice addition! You could add the whatsnew and it is good to merge.

@qbarthelemy
Copy link
Member Author

Yep, but **kwds arguments must also be added
in functions: covariances_EP, covariances_X, block_covariances;
and in classes: ERPCovariances, XdawnCovariances, BlockCovariances, HankelCovariances.

pyriemann/utils/covariance.py Show resolved Hide resolved
pyriemann/utils/covariance.py Show resolved Hide resolved

* 'corr' for correlation coefficient matrix [corr]_,
* 'cov' for numpy based covariance matrix [cov]_,
* 'fpcm' for fixed point covariance matrix [fpcm]_,
Copy link
Member

Choose a reason for hiding this comment

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

I find the name not super informative and reading the PR it's not super clear what it is useful for and why I should use it? Can you add a blob of text somewhere to explain this?

🙏

@agramfort
Copy link
Member

What am I missing here @qbarthelemy ? See the estimate is very different from Id matrix.

image

@qbarthelemy
Copy link
Member Author

Good catch @agramfort !

I had naively translated code from Matlab covariance toolbox https://github.com/alexandrebarachant/covariancetoolbox/blob/master/lib/estimation/FPCov.m
where trace-normalization is missing (Eq(8) in Pascal2005).

It is corrected and I have completed tests.

@qbarthelemy qbarthelemy changed the title Add a new covariance estimator and add **kwds arguments in Covariances Add fixed point covariance estimator and add **kwds arguments in Covariances Jan 27, 2023
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

victory !

Screenshot 2023-01-27 at 23 15 43

Feel free to just add a line in what's new saying that it's a robust estimator.

+1 for MRG

thx @qbarthelemy

doc/whatsnew.rst Outdated Show resolved Hide resolved
@sylvchev
Copy link
Member

Thanks @qbarthelemy for the PR and thank you @agramfort for spotting this!

@sylvchev sylvchev merged commit fb53182 into pyRiemann:master Jan 28, 2023
@qbarthelemy qbarthelemy deleted the fpcm branch January 28, 2023 12:38
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 this pull request may close these issues.

None yet

3 participants