-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MRG: Add joint projection for SSS #5146
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
Conversation
|
Incidentally, I'm not sure the best way to show that this makes much difference in practice. Maybe there will be some difference in the reconstructed signals. I'll think on it. |
Codecov Report
@@ Coverage Diff @@
## master #5146 +/- ##
=========================================
- Coverage 88.8% 88.8% -0.01%
=========================================
Files 401 401
Lines 72687 72748 +61
Branches 12153 12160 +7
=========================================
+ Hits 64552 64604 +52
- Misses 5211 5216 +5
- Partials 2924 2928 +4 |
|
i am sorry I don't understand what this does.
|
|
For data processed with SSS, gives the option to estimate projectors for
mag+grad jointly (i.e. like a single channel type) rather than separately.
|
|
... implemented for now for only a single projector calculation function as
an example for discussion (if people agree it's useful I'll add it to other
projector calculation functions)
|
|
do you think it makes any difference?
|
|
I have the following theoretical arguments:
After playing with |
|
@larsoner this brings us closer to the covariance refactory doesn't it :) I think it's a terrific idea! |
|
@larsoner does this give any advantage in practice apart from speed? |
|
@dengemann please review then if you have time.
my plate is too full to allocate time for this now. My MNE time now is
really
restricted to blockers of the next release.
|
|
I haven't managed to produce a case where |
|
@dengemann do you have time to look / think about this? |
db6bed1 to
04f4408
Compare
|
Okay, rebased, added to Ready for review/merge from my end. |
|
@agramfort this is ready for review/merge (see how it fixes tests). The remaining question is do we deprecate |
|
it's hard for me to judge if meg='joint' should become the default with SSS data without more empirical evaluation of the impact. |
|
At least in terms of the rank numbers, joint does what you'd expect and separate does not. I've shown this now in the tests. But I'm okay not changing the default for a bit. In that case I'll just update what's new |
|
let's just open the what's new for now. Sorry for being many too
conservative :)
… |
larsoner
left a comment
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.
I also updated the naming from meg='joint' to meg='combined' since that's how we have it in our codebase elsewhere (in cov stuff mostly) and it's less confusing with plot_evoked_joint and similar functions
|
@agramfort okay to merge once CIs come back happy? |
|
thx @larsoner |
It's probably not great that
Nprojectors reduce the rank byNfor non-SSS'ed data, but generally by< N(often0) for SSS'ed data. In other words, once SSS makes mag and grad correlated, computing SSP projectors for them separately might not make sense.This PR adds support for
sss='separate'(default, currentmasterbehavior) and'joint'for SSS'ed data. I didn't do anything special about the scaling between mag+grad, but I think it shouldn't matter because of the correlation structure in the data.@agramfort @dengemann WDYT? You can look at the test I added at the bottom of the diff to see how this should work, or recreated here in fuller form in case you want to try it: