MRG: add new compute_rank function #5876
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5876 +/- ##
==========================================
+ Coverage 88.67% 88.76% +0.08%
==========================================
Files 401 401
Lines 72527 72555 +28
Branches 12132 12142 +10
==========================================
+ Hits 64311 64400 +89
+ Misses 5272 5229 -43
+ Partials 2944 2926 -18 |
|
There are still some issues with this new function. The mixed MEG&EEG is still not handled correctly and the first test against ~zero should compare to fpinfo().tiny instead of .eps. I am working on this so don't merge yet. |
|
ok cool. It's far form ready to merge indeed. We'll need to add tests
and add support for scalings to be able to combine sensor types.
|
|
Sure |
larsoner
force-pushed
the
rank_refactor_part3
branch
from
ea361c1 to
5531dc7
Compare
larsoner
force-pushed
the
rank_refactor_part3
branch
from
0116684 to
f4f6f86
Compare
mne/rank.py
Outdated
| number of good channels. If a `Covariance` | ||
| is passed, this can make sense if it has been (possibly | ||
| improperly) regularized without taking into account the true | ||
| data rank. |
There was a problem hiding this comment.
@agramfort this is the docstring that should be scrutinized. Everywhere else there is a rank : XXX will eventually have the same rank : int | None | dict | 'info' | 'full' and description pointing people here for details.
There was a problem hiding this comment.
besides my comment above. Sounds reasonable.
larsoner
changed the title
|
ok then we do this with only one release with a warning that in the next
version int will not be allowed for multisensor types.
sounds good?
… |
|
Yeah that seems safest |
|
that's it for my first pass. Really nice progress @larsoner we should next see how to make sure that rank has the same default for all inverse methods. |
larsoner
changed the title
larsoner
force-pushed
the
rank_refactor_part3
branch
from
382b39e to
776f843
Compare
|
Comments addressed, rebased, and push forced. Unfortunately I found more problems with our I also made explicit where we had to add some hacks to deal with not having #5146. @agramfort ready for one more round of review. If it looks good to you, then after #5753 is in (hopefully today or tomorrow) then I just need to add some |
|
This pull request introduces 1 alert when merging 51a91e1 into fd91ebe - view on LGTM.com new alerts:
Comment posted by LGTM.com |
larsoner
force-pushed
the
rank_refactor_part3
branch
from
51a91e1 to
92adb26
Compare
larsoner
changed the title
|
Ready for review/merge from my end. |
larsoner
force-pushed
the
rank_refactor_part3
branch
3 times, most recently
from
815314e to
094f626
Compare
|
ok that's it for me you'll need to rebase though... |
larsoner
force-pushed
the
rank_refactor_part3
branch
from
094f626 to
64b9c96
Compare
3 participants
the idea is to rely on this single function to compute rank of data / cov
_smart_eighmethodsintrank