Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented May 5, 2020

Digging through our code, I was a bit surprised to see that the tol we apply in compute_rank is an absolute one on the singular values (computed after whatever normalization/scaling you have selected). I've added a tol_kind option that allows treating this as a relative tolerance.

Another option would be to leave tol and add rtol, but that's weird because tol='auto' (and tol='float32') already only behave as a relative tolerance, because they do the same thing that pinv does (scale relative to the max singular val).

Testing with some local MaxFiltered data suggests that it's more robust and flexible than using an absolute tol. I also was able to add it to the dense test_rank.py tests without too many modifications/attempts to find a good value, which I can't say about the absolute version.

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #7736 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7736      +/-   ##
==========================================
- Coverage   90.23%   90.17%   -0.07%     
==========================================
  Files         455      455              
  Lines       84698    84660      -38     
  Branches    13421    13419       -2     
==========================================
- Hits        76429    76341      -88     
- Misses       5402     5444      +42     
- Partials     2867     2875       +8     

@agramfort
Copy link
Member

this works but I would keep in mind that relative will not work always and will typically break if you have a strong artifact that boosts the first eigenvalue (I've see this before)

@larsoner
Copy link
Member Author

Indeed it can break in that case. My use case is always computing covariances on preprocessed data with such artifactual segments removed via SSP or annotations, so less of an issue there.

@larsoner
Copy link
Member Author

but I would keep in mind that relative will not work always

Also it's important to note that our default is 'auto', and in this mode it's always relative. So our default is to use a relative computation. We're in a weird situation here where 'auto' was relative but any float was not. Arguably we should maybe make tol_kind='relative' by default, but it seems nicer from a backward compat standpoint not to force that on people, since they might have computations that depend on it. And it's not worth a deprecation cycle to change it I don't think.

The bottom line is that people might need to try different things to get the right rank computed for their data depending on what processing they've done (ICA, SSS, etc.) and this just gives you more flexibility, and more consistent ways of doing so.

@larsoner
Copy link
Member Author

(pushed a commit to add a caveat about the single-large-component problem and rebased since there was a seemingly random failure)

@agramfort agramfort merged commit 426ab6f into mne-tools:master May 12, 2020
@agramfort
Copy link
Member

thx @larsoner

@larsoner larsoner deleted the rank branch May 12, 2020 19:28
larsoner added a commit to larsoner/mne-python that referenced this pull request May 19, 2020
* upstream/master: (74 commits)
  FIX: Correct a bug in find_bads_eog (mne-tools#7797)
  [MRG] split_naming='bids' changes from _part-%d to _split-%d (mne-tools#7794)
  MRG, MAINT, DOC: Remove spyder (mne-tools#7796)
  MAINT: fixes for linkcheck (mne-tools#7762)
  [WIP] Update ieeg data example for ECoG (mne-tools#7768)
  fix examples/tutorials [circle full] (mne-tools#7786)
  MAINT: Clean up VTK and add to pre on Azure (mne-tools#7780)
  ENH: Add matplotlib animation support [skip travis] (mne-tools#7783)
  MRG, API: change out_type default in permutation_cluster_(1samp_)test (mne-tools#7781)
  DOC: docstring fixes (mne-tools#7777)
  MRG, ENH: Add tol_kind option (mne-tools#7736)
  MRG, DOC: Notes about info (mne-tools#7772)
  ENH: Speed up NIRx read without preload (mne-tools#7759)
  Minor plot_raw aes improvement (mne-tools#7770)
  MRG, FIX: Fixes for BEM contours (mne-tools#7763)
  MRG, STY: Fix E741 (mne-tools#7767)
  MRG, ENH - Plot optodes in plot_alignment for fNIRS channels (mne-tools#7747)
  FIX: Update NIH support [skip travis] (mne-tools#7766)
  MAINT: Bump tol for gamma map test (mne-tools#7764)
  MRG, FIX: Fix MRI orientations (mne-tools#7725)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants