MRG, DOC: Make "rank" options in docs more accessible #8707

hoechenberger · 2021-01-07T13:10:12Z

Basically, I just moved the explanation of the different "rank" settings from the Notes section up to the rank parameter, where I think they belong. I also added an info box, suggesting to use rank='info' on Maxwell-filtered data. I had discussed this with @SophieHerbst, but would appreciate feedback from @agramfort and / or @larsoner

@SophieHerbst

Basically, I just moved the explanation of the different "rank" settings from the Notes section up to the `rank` parameter, where I think they belong. I also added an info box, suggesting to use `rank='info'` on Maxwell-filtered data. I had discussed this with @SophieHerbst, but would appreciate feedback from @agramfort and / or @larsoner

larsoner · 2021-01-07T13:22:06Z

mne/utils/docs.py

+    .. note:: For Maxwell-filtered data, you will typically want set this to
+              ``'info'``.


I would say this is actually not true if you use movement compensation

Any suggestion on how to phrase this better?

Background is that empirical rank determination doesn't work properly for @SophieHerbst with the default settings: rank is always equal to number of channels. Only with rank='info' do we get what looks like a more "trustworthy" rank. We figured that we somehow should inform users about this. Maybe we can take an entirely different approach in communication here? I know too little about this stuff to come up with really useful proposals, I'm afraid! (as just demonstrated!)

It might depend on whether Python or Elekta MaxFilter was used (?) but in general what I do is do everything in Python, compute rank on epochs, then pass this as the rank param in all functions (like cov and inverse computation, etc.). I don't usually have trouble with rank when doing this, so we should figure out what the problem is.

For now I would just remove this extra bit of wording (it's an orthogonal issue) and discuss in #4676 what the right thing is to do, probably by sharing files and figuring out what the problems actually are.

mne/rank.py

hoechenberger · 2021-01-07T15:55:33Z

rendered docs of compute_rank: https://24383-1301584-gh.circle-artifacts.com/0/dev/generated/mne.compute_rank.html#mne.compute_rank

rendered docs of make_inverse_operator: https://24383-1301584-gh.circle-artifacts.com/0/dev/generated/mne.minimum_norm.make_inverse_operator.html#mne.minimum_norm.make_inverse_operator

mne/utils/docs.py

drammock · 2021-01-07T16:53:52Z

mne/utils/docs.py

+        number of good channels. If a `~mne.Covariance` is passed, this can
+        make sense if it has been (possibly improperly) regularized without
+        taking into account the true data rank.
+"""


missing explanation of rank = dict(...)

Yes so what does this actually do? What is the use case? Looking at the code, it seems it simply skips the rank estimation for the channel types that are present in the dict? Why do we need that?

What is the use case? Looking at the code, it seems it simply skips the rank estimation for the channel types that are present in the dict? Why do we need that?

Precisely the one I mentioned that I use above: compute rank once on epochs to obtain a rank dict, then pass this rank dict to compute_covariance, make_inverse_operator, etc.

Ok this does make sense for those functions, but clearly not for compute_rank – but we do have support there, part of the public API. I think what should be done is refactor compute_rank such that we move the implementation into a private function, which gets called by compute_rank, make_inverse_operator etc; and compute_rank wouldn't accept a dict anymore, while make_inverse_operator and friends would.

This seems like a bit of overkill / overengineering to deal with a use case that isn't really causing people problems

(and it's even possible someone knows their EEG rank but not MEG, but wants the result to have both because they have both, so doing out = compute_rank(epochs, rank=dict(eeg=59)) might even be a valid use case for example)

(and it's even possible someone knows their EEG rank but not MEG, but wants the result to have both because they have both, so doing out = compute_rank(epochs, rank=dict(eeg=59)) might even be a valid use case for example)

Honestly I think that this is over-engineered. Because my naive approach would have been:

out = compute_rank(epochs.copy().pick_types(meg=True))

Anyway, I've added a doctoring now :)

Honestly I think that this is over-engineered.

Indeed, but the important difference is that this is (perhaps) over-engineered at the user's end, I'm trying to avoid unnecessary code churn and complications at our (MNE maintenance) end. Plus if it's a valid use case (even if over-engineered) then we don't want to break it just to simplify our docstrings a bit.

mne/utils/docs.py

hoechenberger · 2021-01-07T21:23:06Z

Artifact:
https://24403-1301584-gh.circle-artifacts.com/0/dev/generated/mne.compute_rank.html#mne.compute_rank

larsoner · 2021-01-08T01:27:05Z

mne/rank.py

-
-    Notes
-    -----
-    The ``rank`` parameter can be:
-
-    :data:`python:None` (default)
-        Rank will be estimated from the data after proper scaling of
-        different channel types.
-    ``'info'``
-        Rank is inferred from ``info``. If data have been processed
-        with Maxwell filtering, the Maxwell filtering header is used.
-        Otherwise, the channel counts themselves are used.
-        In both cases, the number of projectors is subtracted from
-        the (effective) number of channels in the data.
-        For example, if Maxwell filtering reduces the rank to 68, with
-        two projectors the returned value will be 68.
-    ``'full'``
-        Rank is assumed to be full, i.e. equal to the
-        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.
-
-    .. versionadded:: 0.18


Suggested change

Notes

-----

The ``rank`` parameter can be:

:data:`python:None` (default)

Rank will be estimated from the data after proper scaling of

different channel types.

``'info'``

Rank is inferred from ``info``. If data have been processed

with Maxwell filtering, the Maxwell filtering header is used.

Otherwise, the channel counts themselves are used.

In both cases, the number of projectors is subtracted from

the (effective) number of channels in the data.

For example, if Maxwell filtering reduces the rank to 68, with

two projectors the returned value will be 68.

``'full'``

Rank is assumed to be full, i.e. equal to the

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.

.. versionadded:: 0.18

Notes

-----

.. versionadded:: 0.18

mne/rank.py

larsoner · 2021-01-08T01:28:46Z

Thanks @hoechenberger

hoechenberger · 2021-01-08T07:21:47Z

@drammock @larsoner I'd like to backport this, any objections?

… [skip github][skip azure]

…thub][skip azure] (#8721)

* upstream/master: (66 commits) MRG, ENH: Add infant template downloader (mne-tools#8738) ENH: add reader for NeuroElectrics .nedf files (mne-tools#8734) DOC: improve glossary entry about fiducials (mne-tools#8763) MRG, ENH: Add Report.add_custom_css (mne-tools#8762) BUG, DOC: read_raw_egi didn't support pathlib.Path; update read_raw() docstring (mne-tools#8759) Add "dbs" as new channel type (mne-tools#8739) MRG, VIZ: Fix title position in plot_sensors (mne-tools#8752) MRG: Support for non-FIFF files in Report.parse_folder (mne-tools#8744) MRG, VIZ, FIX: sEEG picking in _prepare_topomap_plot() (mne-tools#8736) DOC: don't use single letter variable name in _compute_forward (mne-tools#8727) WIP: Fix search [skip github] [skip azp] (mne-tools#8742) WIP: Compare Beer-lambert to HOMER (mne-tools#8711) MRG: bump spyder version (mne-tools#8020) FIX anon with IO round trip (mne-tools#8731) fix set_bipolar_reference for Epochs (mne-tools#8728) WIP: Add width argument, reduce default (mne-tools#8725) ENH: Add toggle-all button to Report (mne-tools#8723) fix int/float conversion in nicolet header (mne-tools#8712) MRG, BUG: Fix Report.add_bem_to_section n_jobs != 1 (mne-tools#8713) MRG, DOC: Make "rank" options in docs more accessible (mne-tools#8707) ...

hoechenberger requested review from agramfort and larsoner January 7, 2021 13:11

Remove trailing whitespaces

eb9e3d0

larsoner reviewed Jan 7, 2021

View reviewed changes

mne/rank.py Show resolved Hide resolved

Update [skip azure][skip github]

b7c0701

hoechenberger changed the title ~~DOC: Make "rank" options in docs more accessible~~ MRG, DOC: Make "rank" options in docs more accessible Jan 7, 2021

hoechenberger removed the request for review from agramfort January 7, 2021 15:55

drammock reviewed Jan 7, 2021

View reviewed changes

mne/utils/docs.py Outdated Show resolved Hide resolved

hoechenberger added 7 commits January 7, 2021 20:10

Address some review comments [skip azure][skip github]

e89b738

Add docstring for dict option [skip azure][skip github]

34c8c79

flake [skip azure][skip github]

6a4d67f

Rephrase [skip azure][skip github]

79cda0b

typo [skip azure][skip github]

5ca0802

phrasing [skip azure][skip github]

90bf7ac

Fix doc build [skip azure][skip github]

d716bf2

larsoner reviewed Jan 8, 2021

View reviewed changes

mne/rank.py Show resolved Hide resolved

Update mne/rank.py

7575e47

larsoner reviewed Jan 8, 2021

View reviewed changes

mne/rank.py Outdated Show resolved Hide resolved

Update mne/rank.py

9af59c9

larsoner merged commit 01b8b36 into mne-tools:master Jan 8, 2021

hoechenberger deleted the docs branch January 8, 2021 06:07

hoechenberger added a commit to hoechenberger/mne-python that referenced this pull request Jan 8, 2021

Backport Make "rank" options in docs more accessible (mne-tools#8707)…

2898063

… [skip github][skip azure]

hoechenberger added a commit that referenced this pull request Jan 8, 2021

Backport Make "rank" options in docs more accessible (#8707) [skip gi…

f2caf79

…thub][skip azure] (#8721)

hoechenberger added the backported label Jan 8, 2021

cbrnr pushed a commit to cbrnr/mne-python that referenced this pull request Jan 15, 2021

MRG, DOC: Make "rank" options in docs more accessible (mne-tools#8707)

0f4ea9e

		.. note:: For Maxwell-filtered data, you will typically want set this to
		``'info'``.

Uh oh!

MRG, DOC: Make "rank" options in docs more accessible #8707

MRG, DOC: Make "rank" options in docs more accessible #8707

Uh oh!

Conversation

hoechenberger commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoechenberger commented Jan 7, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoechenberger commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

larsoner commented Jan 8, 2021

Uh oh!

hoechenberger commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hoechenberger commented Jan 7, 2021 •

edited

Loading