MRG: Do regularization in PCA space #5481
Conversation
|
Can you explain what are the benefits exactly? Sounds like you've thought this through and came to some insight. |
|
When doing an inverse if you regularize the full So instead the approach would be to take the |
|
Yes, it makes sense, do you see a big difference in the inverse results? |
|
For SSS'ed data with a lot of trials on |
|
yes it should look better and we should really move in this direction
|
Codecov Report
@@ Coverage Diff @@
## master #5481 +/- ##
==========================================
- Coverage 88.51% 88.46% -0.06%
==========================================
Files 369 369
Lines 69151 69307 +156
Branches 11651 11662 +11
==========================================
+ Hits 61206 61309 +103
- Misses 5090 5118 +28
- Partials 2855 2880 +25 |
larsoner
changed the title
|
Okay I think this is ready for review/merge. We should probably wait for 0.18 just to be safe / have time to test it, though, so I'm going to leave set as WIP until we release. |
larsoner
changed the title
larsoner
changed the title
|
On second thought, I think the tests are pretty convincing, so I'd be happy putting this in 0.17. Let's see if @agramfort @dengemann you are convinced, too :) |
|
EDIT: Moved updated examples text to top issue |
|
Okay I also fixed the EDIT: Also updated the list of examples above to match commit 1283a0a. |
|
Talking to @dengemann to be safe we should probably wait for 0.18 to merge this. So I'll set to WIP for now. (Also apparently Travis is unhappy so I have some work to do anyway.) Also I'm not 100% sure the |
larsoner
changed the title
|
Fixed the "problem" with the |
| """Compute covariance auto mode.""" | ||
| # rescale to improve numerical stability | ||
| _apply_scaling_array(data.T, picks_list=picks_list, scalings=scalings) | ||
| if rank != 'full': |
There was a problem hiding this comment.
Should be a better test. If len(data) == 306 and rank == 306 this block should not be entered.
There was a problem hiding this comment.
A couple of things:
- The check for this would actually be a bit annoying, as you need to then triage based on the various incantations of
rankinput, instead of just letting_smart_eighbe the one place this is handled / estimated. - In principle it should not matter. As long as we do the rotation properly, it should gracefully fall back to identical behavior.
I have now added a test that point (2) holds. It turns out that it did not before (whoops) but it does now!
|
Hmm, it looks like something bad happened to the EEG scaling here It was not that way before the recent rotation PR: I'll look into it. |
larsoner
changed the title
| - Add :func:`mne.io.read_raw_fieldtrip`, :func:`mne.read_epochs_fieldtrip` and :func:`mne.read_evoked_fieldtrip` to import FieldTrip data. By `Thomas Hartmann`_ and `Dirk Gütlin`_. | ||
|
|
||
| - Add ``rank`` parameter to :func:`mne.compute_covariance`, :func:`mne.cov.regularize` and related functions to preserve data rank during regularization by `Eric Larson`_ and `Denis Engemann`_ | ||
|
|
There was a problem hiding this comment.
Can you also add that we now support faster low-rank (PCA-space) computation?
There was a problem hiding this comment.
Pushed a commit, if it's not what you want feel free to give me a more complete suggestion or push a change
I avoided the word "PCA" because it's more about dealing with rank deficiency rather than projecting onto (orthogonal) directions of highest variance
There was a problem hiding this comment.
One could still highlight that the option can make things significantly faster for SSSd data.
There was a problem hiding this comment.
"and speed up computation" is already in there
|
After playing with several datasets my impression is that this PR does not make things worse than they are. Instead for certain SSS'd the default looks now correct, hence things are improved. As noticed before, the speedup is practically meaningful. I'd suggest to merge this and then work on the outstanding issues in #4676 which are not magically addressed by this PR -- just for expectation management. I'll approve and suggest @agramfort to take his tour. |
|
pushed a cosmit. now with the inverse method the inverse problem are automagically solved in low rank space? or we still have square whitener even if rank is low? |
|
This does not change the behavior of inverse operators / LCMV. So it's still stored square IIRC though at some point gets reduced internally (at least for minimum norm) |
|
Your cosmit broke a test |
|
... and somehow that cosmit also had a non-unicode char that broke Python 2. Quite impressive for a +3/-3 commit @agramfort :) |
|
to get your forgiveness (if I may ask) let me merge this one... |
4 participants
We currently do covariance regularization on the full rank data. Thus rank-deficient covariance matrices become full rank. This PR changes it so that regularization is done in the lower-dimensional space, then projected back out, which should preserve the original rank.
This probably has some small impact on full-rank MEG data (e.g., 306 channels with a handful of projs) but the impact is magnified by Maxwell filtering due to the larger rank reduction.
Something is fishy withtheloglikvalues are pretty uniformly-39in the previous tests across all methods, because they all provide the same quality estimates when using low-rank computation. This score was the best score before, so it's reasonable they all achieve it now (assuming the differences before were driven by how well the low-rank-ness was captured by each method).mne.cov.regularize(..., proj=True)gives same result asdiagonal_fixednowmne.cov.regularizeto respect rank with newrankargumentdiagonal_fixedmethod to match those ofmne.cov.regularizeFix some missing FIFF constants, and fix check for missing onesMRG: Fix constants #5691FixMRG: Fix constants #5691joboutput in info formaxwell_filteringto reflect what we actually doUpdated examples (930bee9):
It looks like most did not change. I did change
bst_phantom_elektato usemethod='shrunk', which no longer increases the cov rank to 306 (hooray!).