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] RESS implem sklearn interface #62

Merged
merged 7 commits into from
May 11, 2023
Merged

Conversation

ludovicdmt
Copy link
Contributor

Hello,

I was recently asked to adapt your RESS implementation so it can be compatible with:

  • MNE formatting (n_trials, n_chans, n_samples) (instead of (n_samples, n_chans, n_trials) in the current implem)
  • Sklearn interface: class inherited from BaseEstimator and TransformerMixin instead of function and fit and transform methods

I basically used your implem and just simplified some parts of it: removed the fold/unfold/demean function to use vectorization from Numpy, use sklearn to compute empirical covariance matrices (can be easily adapted to use another covariance matrix estimation method with regul).
I compared the two implementations on randomly generated data and they outputted the same results (max difference about $10${-16}$).

I also have updated accordingly the test_ress.py so it can run with the new data formatting and the transformation of RESS to a class.

meegkit/ress.py Outdated Show resolved Hide resolved
meegkit/ress.py Outdated Show resolved Hide resolved
meegkit/ress.py Show resolved Hide resolved
@nbara
Copy link
Owner

nbara commented Nov 22, 2022

Hello,

I was recently asked to adapt your RESS implementation so it can be compatible with:

  • MNE formatting (n_trials, n_chans, n_samples) (instead of (n_samples, n_chans, n_trials) in the current implem)

I'm a bit ambivalent about this TBH. The convention for most functions in meegkit is (n_samples, n_chans, n_trials).

It's true that MNE compatibility is nice, but this is a breaking change that would likely impact existing code for all users of the toolbox...

  • Sklearn interface: class inherited from BaseEstimator and TransformerMixin instead of function and fit and transform methods

I basically used your implem and just simplified some parts of it: removed the fold/unfold/demean function to use vectorization from Numpy, use sklearn to compute empirical covariance matrices (can be easily adapted to use another covariance matrix estimation method with regul). I compared the two implementations on randomly generated data and they outputted the same results (max difference about 10{-16}$).

👍👍

I also have updated accordingly the test_ress.py so it can run with the new data formatting and the transformation of RESS to a class.

👍

@nbara nbara added the enhancement New feature or request label Nov 22, 2022
@ludovicdmt
Copy link
Contributor Author

Regarding formatting (MNE or this other one) I don't have strong argument in favor.
Here in ISAE (France, Toulouse) we use exclusively MNE for data loading/processing so this motivated the change but it is probably different elsewhere.

The only argument iwould be that is easier to iterate over epochs with the MNE formatting than with samples-first format, idk.

@nbara
Copy link
Owner

nbara commented Nov 24, 2022

HI @ludovicdmt there are still conflicts in ress.py apparently.

Here in ISAE (France, Toulouse) we use exclusively MNE for data loading/processing so this motivated the change but it is probably different elsewhere.

MNE-style ordering (trials-first) makes sense for mostly offline analyses, but sample-first makes more sense when a) there are no trials (it's much easier to consider the last dimension as optional) b) for real-time applications when the data needs to be serialised.

@ludovicdmt
Copy link
Contributor Author

Yep you are right in the buffer for my real time applications, I have data in 2D format.
Though I artificially add a dimension (simulating single trial) to still be able to use some convenient MNE functions but that's probably not the most efficient way...

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #62 (b5e7bb8) into master (501ef5c) will increase coverage by 0.05%.
The diff coverage is 92.30%.

❗ Current head b5e7bb8 differs from pull request most recent head bcebe66. Consider uploading reports for the commit bcebe66 to get more accurate results

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   78.82%   78.87%   +0.05%     
==========================================
  Files          22       22              
  Lines        2342     2367      +25     
==========================================
+ Hits         1846     1867      +21     
- Misses        496      500       +4     
Impacted Files Coverage Δ
meegkit/utils/denoise.py 76.00% <ø> (ø)
meegkit/ress.py 92.59% <92.15%> (-7.41%) ⬇️
meegkit/utils/sig.py 57.83% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nbara nbara changed the title RESS implem with MNE formatting and Sklearn interface [ENH] RESS implem sklearn interface May 11, 2023
@nbara
Copy link
Owner

nbara commented May 11, 2023

Hey @ludovicdmt finally found some time to look into this.

I reverted the MNE ordering, but kept the sklearn style interface

Merging this, thanks!

@nbara nbara merged commit d9326e7 into nbara:master May 11, 2023
3 of 4 checks passed
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.

None yet

2 participants