MRG: add new compute_rank function by agramfort · Pull Request #5876 · mne-tools/mne-python

agramfort · 2019-01-25T18:01:27Z

the idea is to rely on this single function to compute rank of data / cov

get tests passing
unify old and new _smart_eigh methods
deprecate int rank
wait for MAINT: DRY our docstrings #5753, then update docstrings

codecov · 2019-01-26T08:51:26Z

Codecov Report

Merging #5876 into master will increase coverage by 0.08%.
The diff coverage is 92.39%.

@@            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

LauriParkkonen · 2019-01-26T09:43:48Z

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.

agramfort · 2019-01-26T16:22:31Z

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.

mne/rank.py

agramfort · 2019-02-05T19:59:53Z

maybe we look back at #5146 <#5146> after this PR?

…

larsoner · 2019-02-05T20:05:56Z

Sure

larsoner · 2019-02-05T20:25:52Z

This PR (in addition to #4511, where it is much worse) is another example of why #5753 would be a useful time-saving device. I have replaced only some of the rank : with XXX placeholders, and they'll all need to be updated now and anytime we make changes in the future.

larsoner · 2019-02-10T20:04:56Z

mne/rank.py

+        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.


@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.

besides my comment above. Sounds reasonable.

mne/rank.py

mne/tests/test_cov.py

agramfort · 2019-02-11T20:53:21Z

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?

…

larsoner · 2019-02-11T20:55:17Z

Yeah that seems safest

mne/cov.py

agramfort · 2019-02-11T21:01:35Z

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 · 2019-02-12T22:02:01Z

Comments addressed, rebased, and push forced.

Unfortunately I found more problems with our rank having to do with tol. If we compute with tol='auto' meaning "np.float64 tolerance" then most things work, but test_maxfilter_get_rank does not. I added a special way to use the np.float32 tolerance there to work around it.

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 %(rank)s's and merge.

larsoner · 2019-02-12T23:47:54Z

This pull request introduces 1 alert when merging 51a91e1 into fd91ebe - view on LGTM.com

new alerts:

1 for Unused import

Comment posted by LGTM.com

larsoner · 2019-02-13T02:42:08Z

Ready for review/merge from my end.

mne/io/base.py

mne/rank.py

mne/utils/docs.py

agramfort · 2019-02-15T21:00:24Z

ok that's it for me

you'll need to rebase though...

…arkkonen

agramfort mentioned this pull request Jan 26, 2019

Inconsistant rank estimation and API in beamformers #5879

Closed

4 tasks

larsoner reviewed Feb 5, 2019

View reviewed changes

mne/rank.py Outdated Show resolved Hide resolved

larsoner force-pushed the rank_refactor_part3 branch from ea361c1 to 5531dc7 Compare February 5, 2019 20:23

larsoner force-pushed the rank_refactor_part3 branch from 0116684 to f4f6f86 Compare February 6, 2019 07:19

larsoner reviewed Feb 10, 2019

View reviewed changes

larsoner changed the title ~~[WIP] add new compute_rank function~~ RFC: add new compute_rank function Feb 10, 2019