WIP: loose parameter as dict #4312

joewalter · 2017-06-09T14:34:34Z

WIP addressing #2163 and #4308.

Added a new private function, which checks the input to loose and adapts it if necessary (and also converts forward to surface oriented if required). Currently, only loose = None and loose=1. are supported for vol source spaces. Please let me know if loose orientation constraint should also be available for vol source spaces.

codecov-io · 2017-06-09T16:23:24Z

Codecov Report

Merging #4312 into master will decrease coverage by 0.21%.
The diff coverage is 57.18%.

@@            Coverage Diff             @@
##           master    #4312      +/-   ##
==========================================
- Coverage   82.82%   82.61%   -0.22%     
==========================================
  Files         349      349              
  Lines       64424    65196     +772     
  Branches     9915    10153     +238     
==========================================
+ Hits        53361    53860     +499     
- Misses       8324     8540     +216     
- Partials     2739     2796      +57

agramfort · 2017-06-10T07:08:51Z

mne/forward/forward.py


+def _check_loose(forward, loose):
+    """Check loose parameter input."""
+    if is_fixed_orient(forward):


does it mean I always have a warning when working with fixed ori?

I think we should not have a warning when user set loose to None explicitly.

I added and loose is not None, so the warning will now only appear if loose is not None

agramfort · 2017-06-10T14:41:47Z

mne/forward/forward.py

+    """Check loose parameter input."""
+    if is_fixed_orient(forward) and loose is not None:
+        warn('Ignoring loose parameter with forward operator '
+             'with fixed orientation.')


Say to set it to None to avoid this warning

agramfort · 2017-06-10T14:42:01Z

mne/forward/forward.py

+            if not (0 <= loose <= 1):
+                raise ValueError('Loose value should be smaller than 1 and '
+                                 'bigger than 0, or None for not loose '
+                                 'orientations.')


say what you got

@joewalter reminder here

agramfort · 2017-06-10T14:43:34Z

mne/forward/forward.py

+                raise ValueError('Loose value should be smaller than 1 and '
+                                 'bigger than 0, or None for not loose '
+                                 'orientations.')
+            if loose < 1 and not forward['surf_ori']:


I would say

if loose is not None and not forward['surf_ori']:

agramfort · 2017-06-10T14:44:23Z

mne/forward/forward.py

-        orient_prior[np.mod(np.arange(n_sources), 3) != 2] *= loose
-
+    if ((loose is not None) and
+            not all([item in [1., None] for item in loose.values()])):


I think we should force loose=None rather than accepting 1.

agramfort · 2017-06-10T14:45:31Z

mne/minimum_norm/tests/test_inverse.py

                  noise_cov, depth=None)

-    # for free ori inv, loose=None and loose=1 should be equivalent
+    # for free ori inv, loose=None and loose=1. should be equivalent


@Eric89GXL are you ok to force loose=None for non surf ori forward?

larsoner · 2017-06-10T15:23:18Z

Why not just always use float? It seems cleaner to me.

agramfort · 2017-06-10T16:13:01Z

None is more explicit to say "ignore this param"

larsoner · 2017-06-10T16:58:11Z

Maybe from an internal coding perspective. But it seems less explicit to the user in terms of what the effective value / outcome is, and less simple in terms of values accepted. To put it differently, in the future if we have a parameter in another function that varies smoothly between two extremes, where one extreme effectively does nothing, should we always disallow that extreme value and force people to use None?

larsoner · 2017-06-10T17:03:11Z

... For example, any regularization parameter.

larsoner · 2017-06-10T17:06:57Z

The one way I can see it making sense is if for free orientation forward None meant "don't modify", and for fixed orientation None also meant "don't modify", but in that case we shouldn't disallow the equivalent numerical value (0 or 1) in either case.

agramfort · 2017-06-11T07:04:34Z

we shouldn't disallow the equivalent numerical value (0 or 1) in either case.

ok for me but I would document / recommend the use of None.

joewalter · 2017-06-12T10:57:05Z

Do we want to allow a loose orientation constraint for discrete source spaces? IIRC they are used for defining volume source spaces, so I am wondering whether we should treat them similarly to volume source spaces?

agramfort · 2017-06-12T11:13:03Z

I would say no. Loose ori only makes sense for surface oriented source spaces Alex

larsoner · 2017-06-12T12:03:46Z

I would say yes. Discrete shouldn't be used for volumes, volume type should be used for that. IIUC discrete is used for anything custom the user wants, and normals can easily be properly defined there. In fact the only two things the user provides for Discrete type are locations and normals.

agramfort · 2017-06-12T12:14:02Z

ok for me then as long as volume does not use discrete source space

larsoner · 2017-06-12T13:16:50Z

Argh, we do have these lines:

    if mri is not None:
        for s in sp:
            _add_interpolator(s, mri, add_interpolator)
    elif sp[0]['type'] == 'vol':
        # If there is no interpolator, it's actually a discrete source space
        sp[0]['type'] = 'discrete'

So we change vol type to discrete if there is no mri. This seems a bit odd to me, since it's still defined on a volume grid in volume space. But maybe because it's because you can't easily / properly interpolate into a volume for viz...?

I think this use case is likely rare (?), but the case of user-defined discrete source spaces might also be rare. To be safe, I'm okay with forcing free ori for discrete for now -- we can always add support for it later if we find users need it.

joewalter · 2017-06-12T13:22:22Z

Ok, I'll force free orientation with my next commit.

joewalter · 2017-06-12T13:22:59Z

With regard to sparse solvers I thought of normalizing the vector (weight_tan1, weight_tan2, weight_normal) in order not to bias the solution towards sources with free orientation (I'd add a sparse_inv flag, so that nothing changes for MNE-type methods)

larsoner · 2017-06-12T13:31:51Z

I'd add a sparse_inv flag, so that nothing changes for MNE-type methods

Why wouldn't you also want this for MNE methods, at least as an option? That way if I have 1000 fixed-orientation surface source-space points and 1000 free-orientation volume points, you could achieve roughly equivalent activation mapping into both (spatial distribution considerations aside), right?

agramfort · 2017-06-12T13:35:47Z

same concern as Eric. It seems like a bug to work differently with L2 or sparse solvers.

joewalter · 2017-06-12T14:01:09Z

So the variance of a source depends on whether it is a free or fixed orientation source?

joewalter · 2017-06-13T06:58:31Z

When we agree to only support loose or fixed orientation for surface-based source spaces, we can make the handling of loose much simpler, as we don't need the dict (so the check_loose function is much simpler) and we just add an info that loose parameter is ignored for volume and discrete source spaces.

agramfort · 2017-06-13T07:27:06Z

we could do this for now until there is a motivation to have loose for discrete source space. let's keep it simple

larsoner · 2017-06-14T14:53:04Z

Sounds like a plan.

Does this also take care of #4313?

Is it possible/easy to update some example? Even a mixed-source-space MNE example might benefit from it.

agramfort · 2017-06-16T07:12:31Z

mne/forward/forward.py

+                     'orientation.')
+                forward = convert_forward_solution(forward, surf_ori=True)
+        else:
+            raise ValueError('loose value must be None for not loose '


loose must be None for not loose orientations ?

this is unclear what you mean here.

agramfort · 2017-06-16T07:14:22Z

mne/forward/forward.py

+        else:
+            logger.info('Applying loose dipole orientations for surface-based '
+                        'source spaces with loose=%f.' % loose)
+        idx_start = 0


can you add here

assert not is_fixed_orient(forward)

to avoid futur bugs

larsoner

Any time to come back to this one, too? It'll need a (probably annoying) rebase

larsoner · 2017-08-04T05:10:39Z

mne/forward/tests/test_forward.py

+    assert_true(_check_loose(fwd_fixed, 0.2)[1] is None)
+
+    del fwd, fwd_fixed
+    gc.collect()


if you are really worried about memory, convert_forward_solution has copy=False to modify inplace

larsoner · 2017-08-04T05:11:01Z

mne/forward/tests/test_forward.py


+@testing.requires_testing_data
+def test_check_loose():
+    """Test _check_loose


end with period, no newline before """ (we are slowly updating to latest docstring standard)

larsoner · 2017-09-22T14:52:44Z

@joewalter what is left here? Just a rebase and a few minor remaining comments?

larsoner · 2017-09-22T18:00:54Z

I don't think we should do this until 0.16 so we have some time to finish and then test, pushing to 0.16

joewalter · 2017-09-25T06:50:15Z

I think it is a good idea to push this to 0.16 so we have enough time for improvements and tests

agramfort · 2017-09-25T08:49:48Z

let's not close. We'll take some code from here.

joewalter · 2017-09-26T07:02:29Z

I think to make this one easier, we could split it in several PR. So we can faster merge parts of it, making rebasing simpler

agramfort · 2017-09-26T07:51:08Z

yes agreed

larsoner · 2017-09-26T13:21:24Z

Perhaps the first split could take care of #4604. That would be nice to have for 0.15

larsoner · 2018-02-27T03:17:58Z

Removing 0.16 tag

larsoner · 2018-05-16T17:23:24Z

@joewalter any time to come back to this one?

larsoner · 2019-03-27T17:44:06Z

This PR is way out of date at this point. Now that almost all code is refactored to use the same loose and depth paths, we should probably start with a fresh PR for this.

agramfort reviewed Jun 10, 2017

View reviewed changes

This was referenced Jun 14, 2017

[WIP] Mixed Lead Field for a mixed source space #4317

Closed

depth weighting #4318

Closed

agramfort reviewed Jun 16, 2017

View reviewed changes

joewalter added 2 commits August 2, 2017 10:16

improve warnings

f3b267c

disallow loose = None for free orient

cc33f68

joewalter force-pushed the mixed_mne branch from 584804c to cc33f68 Compare August 2, 2017 08:28

joewalter added 5 commits August 2, 2017 10:39

Fix: flake

a516e11

Fix tests

6426f3e

tests for source space

c605d96

added test for mixed stc

21c4805

flake

efffcff

larsoner reviewed Sep 10, 2017

View reviewed changes

larsoner added this to the 0.15 milestone Sep 10, 2017

larsoner mentioned this pull request Sep 15, 2017

MRG: Make depth compensation work for sphere models #4562

Merged

larsoner modified the milestones: 0.15, 0.16 Sep 22, 2017

joewalter closed this Sep 25, 2017

agramfort reopened this Sep 25, 2017

joewalter mentioned this pull request Sep 26, 2017

API: Remove loose=None #4604

Closed

joewalter mentioned this pull request Sep 28, 2017

MRG: Deprecate loose=None #4618

Merged

larsoner removed this from the 0.16 milestone Feb 27, 2018

annapasca mentioned this pull request Nov 27, 2018

mixed source space and pick_ori='vector' #5751

Closed

larsoner closed this Mar 27, 2019

Uh oh!

WIP: loose parameter as dict #4312

WIP: loose parameter as dict #4312

Uh oh!

Conversation

joewalter commented Jun 9, 2017

Uh oh!

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

Uh oh!

Codecov Report

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!

larsoner commented Jun 10, 2017 via email

Uh oh!

agramfort commented Jun 10, 2017 via email

Uh oh!

larsoner commented Jun 10, 2017 via email

Uh oh!

larsoner commented Jun 10, 2017 via email

Uh oh!

larsoner commented Jun 10, 2017 via email

Uh oh!

agramfort commented Jun 11, 2017 via email

Uh oh!

joewalter commented Jun 12, 2017

Uh oh!

agramfort commented Jun 12, 2017 via email

Uh oh!

larsoner commented Jun 12, 2017 via email

Uh oh!

agramfort commented Jun 12, 2017 via email

Uh oh!

larsoner commented Jun 12, 2017

Uh oh!

joewalter commented Jun 12, 2017

Uh oh!

joewalter commented Jun 12, 2017

Uh oh!

larsoner commented Jun 12, 2017

Uh oh!

agramfort commented Jun 12, 2017 via email

Uh oh!

joewalter commented Jun 12, 2017

Uh oh!

joewalter commented Jun 13, 2017

Uh oh!

agramfort commented Jun 13, 2017 via email

Uh oh!

larsoner commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner left a comment

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!

larsoner commented Sep 22, 2017

Uh oh!

larsoner commented Sep 22, 2017

Uh oh!

codecov-io commented Jun 9, 2017 •

edited

Loading