[MRG] ENH coreg-GUI: use high-res head shape if available #3383

christianbrodbeck · 2016-07-05T15:40:18Z

agramfort · 2016-07-05T16:08:33Z

mne/coreg.py

 fid_fname = pformat(bem_fname, name='fiducials')
 fid_fname_general = os.path.join(bem_dirname, "{head}-fiducials.fif")
 src_fname = os.path.join(bem_dirname, '{subject}-{spacing}-src.fif')
+high_res_head_fname = os.path.join(subject_dirname, 'surf', 'lh.seghead')


see what surface is produced by

mne make_scalp_surfaces

it's not the surface that should default to if you assume you ran "mne make_scalp_surfaces"

Typically you should if possible take the high res surface for
coregistration, if it is available. MNE analyze relies on users linking
them correctly ...

On Tue, Jul 5, 2016 at 5:08 PM Alexandre Gramfort notifications@github.com
wrote:

In mne/coreg.py
#3383 (comment):

@@ -39,6 +39,7 @@
fid_fname = pformat(bem_fname, name='fiducials')
fid_fname_general = os.path.join(bem_dirname, "{head}-fiducials.fif")
src_fname = os.path.join(bem_dirname, '{subject}-{spacing}-src.fif')
+high_res_head_fname = os.path.join(subject_dirname, 'surf', 'lh.seghead')

see what surface is produced by

mne make_scalp_surfaces

it's not the surface that should default to if you assume you ran "mne
make_scalp_surfaces"

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/mne-tools/mne-python/pull/3383/files/f285ec969a9dc99a8fd3fe827184990a3b6ac382#r69591426,
or mute the thread
https://github.com/notifications/unsubscribe/AB0fitjdp-7vywfzwGX6TDZFyDUwSh4kks5qSoGBgaJpZM4JFP8_
.

@agramfort lh.seghead is produced by FreeSurfer's mkheadsurf (the first step of $ mne make_scalp_surfaces) and seems to contain the same information as head-dense-bem.fif (as in np.allclose). My next plan was to generate lh.seghead when it is not present with a call to FreeSurfer. Is there a reason to create an additional file with the same information? If there is a use case for modifying head-dense.fif we could look for head-dense.fif before lh.seghead?

@dengemann that is the intention behind this PR

christianbrodbeck · 2016-07-06T02:11:52Z

Would we also prefer the high resolution head shape for FSAverage? I don't know the details of how it is computed but it showed up when I pointed the GUI to fsaverage (i.e., the GUI ran $ mkheadsurf on it). The plot does seem to suggest that the LPA and RPA fiducials are somewhat misplaced. Is that an artifact of the fsaverage creation?

dgwakeman · 2016-07-06T02:19:36Z

This is just intuition, but I would say no, since it is an average (and a particularly special average at that: I was even surprised mkheadsurf would produce something vaguely head shaped). I think keeping it smooth gives an appropriate amount of play and smoothness for any unrecommended case where someone is using fsaverage, and it is less likely to throw off the ICP alignment via overfitting to a non-individual's brain. my 2 cents

christianbrodbeck · 2016-07-06T15:10:04Z

So then I'm wondering how this should be reflected in the GUI. Should we detect an average brain and only show the normal head even though high resolution head is checked? Or should we just include advice on it? Or have a second checkbox that says something like "also use high resolution brain for fsaverage" and is off by default?

dgwakeman · 2016-07-06T15:15:40Z

Is there anything that can tell that a subject is not the genuine subject?, we could just uncheck the box and put a note saying for non-individual MRIs we don't recommend using the high resolution surface (assuming people agree, I haven't tested, it just seems like there is potential for find "strange minima").

christianbrodbeck · 2016-07-06T15:17:57Z

I don't know if there is a general rule, fsaverage could be detected based on its name

agramfort · 2016-07-06T15:56:58Z

what is the reason why the high res head is bad for non individual MRIs?

dgwakeman · 2016-07-06T17:26:24Z

Because the two features the high res is most useful for (the ears and nose) are highly variable in their shape (and the ears can have different positions). People can easily overfit or worry about their match based on the fiducials. The advantage of the high res, is that I can fit the points right in the valley between the crus of helix and the tragus (where I digitize) and the nasion. In the case of using an MRI of someone you didn't scan, It isn't important for them to be at the right spot there. What matters most is that the EEG sensors are in the correct position. If someone uses for example the keep nasion option, on a person's data who has an odd shaped nose then, they will be hurting their ability to get a good fit. Again this is intuition, so feel free to ignore

codecov-io · 2016-07-07T02:19:55Z

Current coverage is 86.63%

Merging #3383 into master will decrease coverage by <.01%

@@             master      #3383   diff @@
==========================================
  Files           337        337          
  Lines         58119      58151    +32   
  Methods           0          0          
  Messages          0          0          
  Branches       8861       8867     +6   
==========================================
+ Hits          50357      50381    +24   
- Misses         5081       5088     +7   
- Partials       2681       2682     +1

Powered by Codecov. Last updated by d9a2d09...7e83d1c

agramfort · 2016-07-07T09:50:02Z

mne/coreg.py

+        mri = 'T1'
+    run_subprocess('source $FREESURFER_HOME/FreeSurferEnv.sh; ' +
+                   'mkheadsurf -subjid ' + subject + ' -srcvol ' + mri,
+                   env=env, shell=True)


I don't like this much as it means computation is done by the GUI while we should document how it should be done with mne command. If you don't agree with how the mne command does stuff the fix should be there.

I have no issue with the mne command, it does the same thing. :)

But what should the GUI do if the high resolution head is not present? Show an error message that asks the user to leave the GUI, run a command for each subject in the command line, and then restart the GUI? I feel like we discussed a similar issue with creating the average subject, where we ended up preferring that the GUI should automatically do a task for the user if it is deterministic...

On Jul 7, 2016, at 5:50 AM, Alexandre Gramfort notifications@github.com wrote:

In mne/coreg.py #3383 (comment):

fs_home)

env = os.environ.copy()

env['SUBJECTS_DIR'] = subjects_dir

env['SUBJECT'] = subject

env['FREESURFER_HOME'] = fs_home

env['PATH'] = os.path.join(fs_home, 'bin') + ':' + env['PATH']

subj_path = os.path.join(subjects_dir, subject)

if os.path.exists(os.path.join(subj_path, 'mri', 'T1.mgz')):

mri = 'T1.mgz'

else:

mri = 'T1'

run_subprocess('source $FREESURFER_HOME/FreeSurferEnv.sh; ' +

'mkheadsurf -subjid ' + subject + ' -srcvol ' + mri,

env=env, shell=True)
I don't like this much as it means computation is done by the GUI while we should document how it should be done with mne command. If you don't agree with how the mne command does stuff the fix should be there.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/mne-tools/mne-python/pull/3383/files/5a207026f41c5c979f0480cc3294ceac3ef875fc#r69880037, or mute the thread https://github.com/notifications/unsubscribe/AAI5a9xUc4p948qh_gFaJxR5PSicOkNBks5qTMvKgaJpZM4JFP8_.

Show an error message that asks the user to leave the GUI, run a command
for each subject in the command line,

yes

my 2c

(Not needed anymore because prepare_bem_model is now done with Python code)

christianbrodbeck · 2016-07-14T03:21:24Z

@agramfort

dgwakeman · 2016-07-14T14:06:45Z

@christianbrodbeck what do you think the right approach is for the average?

I think the image you provided of fsaverage is a good example of my argument, the fiducials aren't lined up with a good point on the ear or the nasion, and I think that is a good thing (because of my earlier argument)

christianbrodbeck · 2016-07-14T14:16:30Z

I updated the screen shot at the top of the PR (see the check box in the top left field).

@dgwakeman I don't like the idea of ignoring the checkbox for fsaverage and prevent users even if they intentionally want to display the high res head. I wonder whether there should be a second checkbox after the "high res head" button that says "use standard for fsaverage"? Although there is the question of whether fsaverage is actually the brain that the majority of average brain users use (if we use the name to detect it and if there is not a method to detect average brains in general). Otherwise I think it should just be documented that for fsaverage the high res head is less appropriate...?

dgwakeman · 2016-07-14T14:30:27Z

Sounds reasonable, I guess the other key would be to have that in the example for coregistering/scaling

christianbrodbeck · 2016-07-14T15:30:54Z

I suppose viz.plot_trans should also use the high resolution head if available, should I add this to this PR or make a separate one?

larsoner · 2016-07-14T15:31:54Z

Separate PR would be better

christianbrodbeck · 2016-07-14T15:46:18Z

mne/gui/_coreg_gui.py

-                   "mne_prepare_bem_model tool will fail. Please install "
-                   "MNE.")
-            warning(None, err, "MNE_ROOT Not Set")
-


This was left over from #3332

christianbrodbeck · 2016-07-19T18:32:29Z

This is ready from my end unless there are suggestions for modifying the functionality. Currently this adds a checkbox for using high res head shapes (see first post, top left of the GUI), and will just display an error message if the high res head shape can not be found.

larsoner · 2016-07-19T18:35:16Z

@christianbrodbeck I think @jona-sassenhagen had a few issues when trying to use the GUI, I assume those should be separate issues rather than fix them in this PR?

christianbrodbeck · 2016-07-19T18:37:46Z

Yes I think #3432 is separate, what were other issues?

larsoner · 2016-07-19T18:42:57Z

Not sure... maybe that was the only one. Didn't realize there was an issue open already.

This LGTM, @jona-sassenhagen since you're in coreg mode can you give it a try?

jona-sassenhagen · 2016-07-20T20:24:20Z

Not specific to this PR, but on my Mac, I get ...

/Users/jona/anaconda3/envs/py2/lib/python2.7/site-packages/pyface/ui/wx/init.py:25: wxPyDeprecationWarning: Using deprecated class PySimpleApp.
  _app = wx.PySimpleApp()
/Users/jona/anaconda3/envs/py2/lib/python2.7/site-packages/pyface/ui/wx/init.py:29: wxPyDeprecationWarning: Call to deprecated item 'InitAllImageHandlers'.
  wx.InitAllImageHandlers()

christianbrodbeck · 2016-07-20T20:26:38Z

@jona-sassenhagen you should use QT backend, run $ export ETS_TOOLKIT="qt4". If you started it from the Terminal with $ mne coreg you should have gotten a message about that.

jona-sassenhagen · 2016-07-20T20:31:58Z

Can't use QT for now because there's a problem with libpng and QT dependencies on Conda and working through them would be way too much work to be honest. I can try again when new versions of libpng or QT arrive on Conda.
Still, when I try, I actually get this funny error message (instead of the error about QT I usually get when I try to force the QT backend):

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-b88511442c68> in <module>()
----> 1 mne.gui.coregistration()

/Users/jona/anaconda3/envs/py2/lib/python2.7/site-packages/mne-0.13.dev0-py2.7.egg/mne/gui/__init__.pyc in coregistration(tabbed, split, scene_width, inst, subject, subjects_dir)
     66     view = _make_view(tabbed, split, scene_width)
     67     gui = CoregFrame(inst, subject, subjects_dir)
---> 68     gui.configure_traits(view=view)
     69     return gui
     70

AttributeError: 'CoregFrame' object has no attribute 'configure_traits'

However, that also happens on master.

christianbrodbeck · 2016-07-20T20:39:08Z

Hm that might be due to our catching the ImportError internally for testing... As far as I know TraitsUI is only really compatible with QT backend (and possible the ancient wx 2.8?) so I am not sure if you can get it to run without QT...

christianbrodbeck · 2016-07-20T20:39:40Z

Although on conda you could create a virtualenv...

jona-sassenhagen · 2016-07-20T20:39:47Z

Maybe not really worth investigating until I have a working pyqt.

christianbrodbeck · 2016-07-20T20:40:44Z

For me something like this works:

$ conda create -n mayavi python=2.7 anaconda mayavi

jona-sassenhagen · 2016-07-20T20:42:54Z

That's how I've set up the env I'm using right now (I'm not using py2 usually). The problem is a libpng conflict: gpilab/framework#3

I could in principle set up different versions of either dep, but that'd be effort ...

christianbrodbeck · 2016-07-20T21:16:50Z

Should we try to avoid silencing those import errors?

larsoner · 2016-08-09T19:51:56Z

Should we try to avoid silencing those import errors?

Is this still an open question? What import errors do you mean?

christianbrodbeck · 2016-08-10T12:32:31Z

@Eric89GXL I meant the

try:
    from mayavi import x ...
except ImportError:
    x = None

pattern, because for people that don't have Mayavi installed this hides the true reason the GUI fails, Instead displaying something like

AttributeError: 'CoregFrame' object has no attribute 'configure_traits' ```

I think we originally added those to enable testing of subcomponents of the GUIs without Mayavi. Since some of the automatic testing environment now support Mayavi we could just make sure that on others modules that import Mayavi don't get imported?

larsoner · 2016-08-15T13:54:40Z

Yeah that sounds reasonable to me. Nest all mayavi and related imports if necessary

christianbrodbeck · 2016-08-18T03:33:25Z

Ok I will do that once the other GUI PRs are merged to avoid rebasing

larsoner · 2016-08-18T12:56:12Z

Which PR do you mean? #3463 is the only one marked MRG but currently needs a rebase

christianbrodbeck · 2016-08-22T17:30:04Z

@Eric89GXL Yes I meant #3463 and the current one this discussion is in.

larsoner · 2016-08-22T17:35:47Z

Okay agreed, let's fix the other issues from @jona-sassenhagen in another PR then

larsoner · 2016-08-22T17:36:24Z

Thanks @christianbrodbeck

agramfort reviewed Jul 5, 2016
View reviewed changes

christianbrodbeck force-pushed the coreg-hsp branch from aff7b3c to 7bf0268 Compare July 6, 2016 15:11

agramfort reviewed Jul 7, 2016
View reviewed changes

christianbrodbeck and others added 10 commits July 13, 2016 10:04

ENH coreg-GUI: use high-res head shape if available

5dfccf4

ENH: check addition high-res head shape paths

c96c7ad

FIX coreg-gui scene_width parameter

cb418e6

ENH coreg-gui: make high-res head optional

23bc951

ENH coreg-gui: create high res head shape if it is not available

54b2973

RF finding high res heads

81c113e

ENH coreg-gui: precompute high resolution heads

7bdfa72

FIX utils.run_subprocess(): command can be str

3894761

fix TEST

af20230

fixes

7c674bf

christianbrodbeck force-pushed the coreg-hsp branch from 279775b to c3141a3 Compare July 13, 2016 14:06

christianbrodbeck added 2 commits July 13, 2016 11:11

TEST

384e8cf

Skip scaled MRI-subjects

7f442b7

christianbrodbeck force-pushed the coreg-hsp branch 2 times, most recently from 958ae0c to c95b665 Compare July 14, 2016 00:24

Error message instead of making high res head

1da57a2

christianbrodbeck force-pushed the coreg-hsp branch from c95b665 to 1da57a2 Compare July 14, 2016 02:29

FIX coreg-gui: don’t ask for MNE_ROOT

44f6363

(Not needed anymore because prepare_bem_model is now done with Python code)

christianbrodbeck reviewed Jul 14, 2016
View reviewed changes

larsoner merged commit 4aa3f3a into mne-tools:master Aug 22, 2016

christianbrodbeck deleted the coreg-hsp branch August 23, 2016 20:01

christianbrodbeck mentioned this pull request Oct 6, 2016

[MRG+1] move trait import out of try block #3648

Merged

Uh oh!

[MRG] ENH coreg-GUI: use high-res head shape if available #3383

[MRG] ENH coreg-GUI: use high-res head shape if available #3383

Uh oh!

Conversation

christianbrodbeck commented Jul 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort Jul 5, 2016

Choose a reason for hiding this comment

Uh oh!

dengemann Jul 5, 2016

Choose a reason for hiding this comment

Uh oh!

christianbrodbeck Jul 5, 2016

Choose a reason for hiding this comment

Uh oh!

christianbrodbeck commented Jul 6, 2016

Uh oh!

dgwakeman commented Jul 6, 2016

Uh oh!

christianbrodbeck commented Jul 6, 2016

Uh oh!

dgwakeman commented Jul 6, 2016

Uh oh!

christianbrodbeck commented Jul 6, 2016

Uh oh!

agramfort commented Jul 6, 2016 via email

Uh oh!

dgwakeman commented Jul 6, 2016

Uh oh!

codecov-io commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 86.63%

Uh oh!

agramfort Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

christianbrodbeck Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

agramfort Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

christianbrodbeck commented Jul 14, 2016

Uh oh!

dgwakeman commented Jul 14, 2016

Uh oh!

christianbrodbeck commented Jul 14, 2016

Uh oh!

dgwakeman commented Jul 14, 2016

Uh oh!

christianbrodbeck commented Jul 14, 2016

Uh oh!

larsoner commented Jul 14, 2016

Uh oh!

christianbrodbeck Jul 14, 2016

Choose a reason for hiding this comment

Uh oh!

christianbrodbeck commented Jul 19, 2016

Uh oh!

larsoner commented Jul 19, 2016

Uh oh!

christianbrodbeck commented Jul 19, 2016

Uh oh!

larsoner commented Jul 19, 2016

Uh oh!

jona-sassenhagen commented Jul 20, 2016

Uh oh!

christianbrodbeck commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jona-sassenhagen commented Jul 20, 2016

Uh oh!

christianbrodbeck commented Jul 20, 2016

Uh oh!

christianbrodbeck commented Jul 20, 2016

Uh oh!

jona-sassenhagen commented Jul 20, 2016

Uh oh!

christianbrodbeck commented Jul 5, 2016 •

edited

Loading

codecov-io commented Jul 7, 2016 •

edited

Loading

christianbrodbeck commented Jul 20, 2016 •

edited

Loading