[MRG] Simplify cov code path by agramfort · Pull Request #4601 · mne-tools/mne-python

agramfort · 2017-09-25T13:51:50Z

a first attempt to give love to cov code by simplifying code path

you can see that dipole fit and LCMV use compute_whitener while make_inverse_operator is using _get_whitener so does not use scalings and does not back project (using eigvec).

this need further thinking...

.. note:: how a lunch with @dengemann ends up :)

agramfort · 2017-09-25T13:52:48Z

+#     scalings = _handle_default('scalings_cov_rank', scalings)
+#     W, _, rank = compute_whitener(noise_cov, info, picks=picks, rank=rank,
+#                                   scalings=scalings, return_rank=True)
+#     return W, rank


I'll remove this if travis is happy

larsoner · 2017-09-25T15:11:33Z

-    scale = _get_whitener_data(info, scale, meg_picks, verbose=False)[0]
+
+    scalings = _handle_default('scalings', None)
+    # WTF is the whitener called scale ! cc @larsoner...


yes because all it does is scale each channel (it's a diagonal matrix), feel free to change if you want

larsoner · 2017-09-25T15:12:12Z


        .. versionadded:: 0.15
+    diag : bool
+        Use a diagonal approximation of the noise covariance.


versionadded

wait, why do we need this? Why not use make_ad_hoc_cov or noise_cov.as_diag()?

it's used in whiten_evoked which also has a diag option. I think it's cleaner to do all kind of whitening in the same function.

whiten_evoked shouldn't have the option, either :) Rather than propagate this error, I'd rather deprecate it, or do the logic inside whiten_evoked. Does it really save that much work to have this option?

larsoner · 2017-09-25T15:16:15Z

    scale = make_ad_hoc_cov(info, verbose=False)
-    scale = _get_whitener_data(info, scale, meg_picks, verbose=False)[0]
+
+    scalings = _handle_default('scalings', None)


you shouldn't need to do this -- this is only used for cov rank internally by compute_whitener, which will easily pass for this diagonal matrix even if it's not a perfect match for the values make_ad_hoc_cov uses.

larsoner · 2017-09-25T15:19:27Z

    # whitener = np.dot(whitener, cov['eigvec'])

-    whitener, rank = _get_whitener_data(info, cov, picks, verbose=False)
+    scalings = _handle_default('scalings', None)


Same here, I doubt we really need to do this one. I'd consider it a bugfix not to, as it's a better match with what all other solvers do. Can you try not doing it and see if tests still pass? If they don't it might mean we have some problems with our cov rank scale factors.

dengemann · 2017-09-27T21:42:20Z

        coils = _prep_mf_coils(info)
-    scale = make_ad_hoc_cov(info, verbose=False)
-    scale = _get_whitener_data(info, scale, meg_picks, verbose=False)[0]
+    diag_cov = make_ad_hoc_cov(info, verbose=False)


this is much clearer.

dengemann · 2017-09-27T21:44:50Z

+    #   Omit the zeroes due to projection
+    eig = noise_cov['eig']
+    nzero = (eig > 0)
+    n_nzero = sum(nzero)


maybe we can avoid these matlab style names like n_zero.

Why not:

n_zero = sum(eig > 0)

My bad, just forget what I said. Let's keep it as is.

However, I think replacing *nzero with *non_zero would be a good idea.

non_zero = (eig > 0) n_non_zero = sum(non_zero) # and so on ...

dengemann · 2017-09-27T21:48:36Z

+        noise_cov = noise_cov.copy()
+        noise_cov['data'] = np.diag(np.diag(noise_cov['data']))
+
+    scalings = _handle_default('scalings_cov_rank', scalings)


I think this line is not needed any longer. It anyways reappears further down the road in the cov rank estimation function (which we will probably abolish in the future).

I will keep it for now so you can test I can safely remove it. you need to redo the evaluation you did when you added this.

dengemann · 2017-09-27T21:49:28Z

+                                          pca=False, scalings=scalings)
+
+    # Do the back projection
    W = np.dot(noise_cov['eigvec'].T, W)


did we want to make this an option?

What about this one?

arfff true. But then I change the behavior of existing code and I need more complete testing. LCMV and dipole fit have now used the back projection...

I suggest to add it in another PR what would change the numerics.

I don't want to break anything just before the release without full testing

codecov-io · 2017-09-27T22:29:03Z

Codecov Report

Merging #4601 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4601      +/-   ##
==========================================
- Coverage   87.77%   87.71%   -0.07%     
==========================================
  Files         351      351              
  Lines       66244    66443     +199     
  Branches    10272    10326      +54     
==========================================
+ Hits        58148    58281     +133     
- Misses       5159     5206      +47     
- Partials     2937     2956      +19

larsoner

LGTM +1 for merge

agramfort · 2017-09-28T20:37:00Z

good to go from my end when green

dengemann · 2017-09-28T21:27:31Z

I am surprised the code passes like this. Got to take another look. To me all could be done using one function with optional backprojection.

…

On Thu, 28 Sep 2017 at 22:46, Alexandre Gramfort ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mne/cov.py <#4601 (comment)>: > - eig = noise_cov['eig'] - nzero = (eig > 0) - W[nzero, nzero] = 1.0 / np.sqrt(eig[nzero]) - # - # Rows of eigvec are the eigenvectors - # - W = np.dot(W, noise_cov['eigvec']) + if diag: + noise_cov = noise_cov.copy() + noise_cov['data'] = np.diag(np.diag(noise_cov['data'])) + + scalings = _handle_default('scalings_cov_rank', scalings) + W, noise_cov, n_nzero = _get_whitener(noise_cov, info, ch_names, rank, + pca=False, scalings=scalings) + + # Do the back projection W = np.dot(noise_cov['eigvec'].T, W) arfff true. But then I change the behavior of existing code and I need more complete testing. LCMV and dipole fit have now used the back projection... I suggest to add it in another PR what would change the numerics. I don't want to break anything just before the release without full testing — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4601 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0fijmyp9V2qcw4ql--naIRsr9VjPu_ks5snAWagaJpZM4PixEU> .

agramfort · 2017-09-29T09:33:23Z

@dengemann ok to merge this one and do the rest in a subsequent PR?

larsoner · 2017-09-29T13:12:12Z


        .. versionadded:: 0.15
+    diag : bool
+        Use a diagonal approximation of the noise covariance.


wait, why do we need this? Why not use make_ad_hoc_cov or noise_cov.as_diag()?

larsoner · 2017-09-29T13:17:29Z

-    if diag:
-        noise_cov = noise_cov.copy()
-        noise_cov['data'] = np.diag(np.diag(noise_cov['data']))
+    scalings = _handle_default('scalings', None)


It looks like this code path (for whiten_evoked) previously used:

_get_whitener_data

Which in turn used:

scalings = _handle_default('scalings_cov_rank', scalings)

but now you have it just do:

scalings = _handle_default('scalings', None)

Shouldn't this still be scalings = _handle_default('scalings_cov_rank', scalings) here?

If so, I don't think you even need it because compute_whitener does this default check internally on line 1641.

agramfort · 2017-09-29T21:25:33Z

done let's avoid an unnecessary deprecation cycle here. I think it has some educational value to see the data whitened with diag noise cov so let's keep this option explicit

agramfort commented Sep 25, 2017

View reviewed changes

larsoner reviewed Sep 25, 2017

View reviewed changes

agramfort added 6 commits September 27, 2017 20:34

remove code dupe

fea435c

remove _get_whitener_data

9c27058

fix example

c1077b3

address comments and ideally fix travis

1d6d121

remove scalings

6ab4ff5

function missing in reference

d93046a

agramfort force-pushed the simplify_cov_code_path branch from f35a894 to d93046a Compare September 27, 2017 18:41

dengemann reviewed Sep 27, 2017

View reviewed changes

larsoner approved these changes Sep 28, 2017

View reviewed changes

address @dengemann's comments

0315ed8

agramfort changed the title ~~[WIP] Simplify cov code path~~ [MRG] Simplify cov code path Sep 28, 2017

larsoner added this to the 0.15 milestone Sep 29, 2017

larsoner reviewed Sep 29, 2017

View reviewed changes

agramfort added 2 commits September 29, 2017 21:35

remove get scalings

b361f42

address comments

b294221

larsoner merged commit ce2486c into mne-tools:master Sep 30, 2017

larsoner deleted the simplify_cov_code_path branch September 30, 2017 00:03

karpovantonme mentioned this pull request Aug 3, 2026

Align _AbstractRenderer.tube with the PyVista implementation #14125

Merged

Uh oh!

Uh oh!

Conversation

agramfort commented Sep 25, 2017 • 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!

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!

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!

codecov-io commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

agramfort commented Sep 28, 2017

Uh oh!

dengemann commented Sep 28, 2017 via email

Uh oh!

agramfort commented Sep 29, 2017

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!

agramfort commented Sep 29, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

agramfort commented Sep 25, 2017 •

edited

Loading

codecov-io commented Sep 27, 2017 •

edited

Loading