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
Implementation of Block Covariance estimation #154
Conversation
Thanks for the PR, this is a nice code. There is some linting errors in CI, that you coud see by running Could you update what's new page in |
Co-authored-by: Sylvain Chevallier <sylvain.chevallier@uvsq.fr>
Ah yes, sorry for not checking before. |
Obviously, block covariance processing is interesting after filter-bank, like in SSVEP paradigm. But I think that the suggested implementation is sub-optimal. After a filter-bank with non-overlapped frequency bands, you usually build an extended version of the input signal with filter outputs concatenated along channel axis.
However, this type of matrices has a uselessly too high dimension, leading to: If you consider two block-diagonal matrices:
then, you can see that:
processing only low-dimensional matrices.
My two cents:
I hope that’s clear... |
I had thought about that as well, but I couldn't find a reasonable solution that didn't include writing and own BlockCovariance class and rewriting all things like eigenvalue decomposition etc for that. Interestingly, although it is stored as a n*n matrix, the runtime for EVD and such reduces to about a half from the full covariances (at least in some toy examples that I wrote up). So although this is definitely far from optimal code, it could have some usefulness. But if you think that this shouldn't be included like this, I could try to figure out a way to make it more efficient. |
Yes, apart from the heavy solution to write dedicated classes for SPD and blockSPD matrices, which increase code complexity for an unclear efficiency gain, I see another possibility. This is to use scipy sparse matrices and there is even block diagonal matrix structure. But the efficiency gain is still unclear as most of our codebase rely on numpy's linear algebra, so the whole codebase requires a major rewriting. I agree with your general remark @qbarthelemy but I think we could use the code of this PR as it is, especially if this could save some runtime when performing EVD. |
@gabelstein This seems good to me one you fixed the line length for the CI; ok to merge after that? |
Jup, looks good. |
Could you correct the CI error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for me to integrate the current implementation.
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabelstein , can you check last modifications?
Everything looks good. |
Thx @gabelstein for the contribution! |
This includes block diagonal covariance estimation.
Useful for tasks where multiple band-pass filters are applied to the original data and covariance between frequencies do not hold information.