[MRG] convert surface using python. #3273

jaeilepp · 2016-05-31T11:54:18Z

Closes #3107
closes #3281
For most parts I had no idea what I was doing, but it seems to give similar results.

jaeilepp · 2016-05-31T11:56:29Z

mne/bem.py

+    trans[3][3] = 1.
+    trans = Transform(FIFF.FIFFV_COORD_UNKNOWN, FIFF.FIFFV_COORD_MRI, trans)
+    surf_out = transform_surface_to(surf_in, FIFF.FIFFV_COORD_MRI, trans)
+    write_surface(fname_out, surf_out['rr'], surf_out['tris'])


Here I saved 'tris', but there is also 'use_tris'. What's the difference?

I don't think similar results are safe enough. Matti had probably been careful about preserving topology and the like, can you base the changes on the C code? It's the safest option. tris are the original undecimated surface and use_tris come from the decimated version, see how they are used in the source space code. And please document it as you figure it out, I probably should have done it originally :(

can you base the changes on the C code?

This is basically a copy paste of the c-code apart from the calls to the existing python functions. I suppose they do the same thing.

Okay cool, that should work (especially if you unit-test against files processed by both Python and C)

Now that I look again, I don't think this file should actually write out the result, just return it. Better separation of functionality to have the write_surface called afterward.

codecov-io · 2016-05-31T12:22:57Z

Current coverage is 77.80%

Merging #3273 into master will decrease coverage by 8.66%

@@             master      #3273   diff @@
==========================================
  Files           335        335           
  Lines         57322      57440    +118   
  Methods           0          0           
  Messages          0          0           
  Branches       8715       8733     +18   
==========================================
- Hits          49567      44692   -4875   
- Misses         5132      10634   +5502   
+ Partials       2623       2114    -509

Powered by Codecov. Last updated by ea83b84...0196b28

jaeilepp · 2016-05-31T12:23:08Z

mne/bem.py

+    from .surface import _read_surface_geom, write_surface
+    header = _get_mgz_header(T1_mgz)
+    surf_in = _read_surface_geom(fname_in)
+    surf_in['coord_frame'] = FIFF.FIFFV_COORD_MRI


And this is FIFFV_COORD_UNKNOWN in the c-code, but it throws an error in python.

I think this is actually more correct as it is in MRI coords anyway (right?)

actually looking below I think it isn't MRI coords...

i.e. it shuold be FIFFV_COORD_UNKNOWN and there must be some bug below

agramfort · 2016-05-31T13:58:23Z

how does the output compare with the output of the C code?

jaeilepp · 2016-06-01T05:40:54Z

The results look similar, but I'm not sure if it's even transforming anything for the data I have. Trying to set up a proper c-environment now so I can isolate stuff better.

jaeilepp · 2016-06-02T12:02:40Z

Okay, Now that I've actually been able to debug the c-code, I realize that most of this is unnecessary. What confuses me now is the --replacegeom parameter that is fed to the c-function. AFAIK, it only updates the tags from the T1.mgz. There isn't really much documentation on the tags (see https://surfer.nmr.mgh.harvard.edu/fswiki/FsTutorial/MghFormat) and our sample data doesn't seem to have any significant information available. Do you think it would be okay to leave out the tag updating and just do the same as leaving out the --mghmri and --replacegeom params? AFAIK the tag information isn't even used in python code elsewhere.

agramfort · 2016-06-02T12:11:36Z

@mshamalainen any objection? idea? on my side whatever works :)

larsoner · 2016-06-02T12:34:38Z

That's fine, we can always add the options later. So long as they are not the defaults we should be fine.

jaeilepp · 2016-06-07T11:41:01Z

So what do we want to do with metadata in nipy/nibabel#456? Maybe just add a read_metadata option to read_surface and metadata option to write_surface.

agramfort · 2016-06-07T12:52:18Z

nipy/nibabel#458 I just took 1h to play...

jaeilepp · 2016-06-07T15:23:00Z

Why is mne-python not using nibabel for reading the surfaces? I see the only difference is basically NEW_QUAD_MAGIC. Couldn't that be added to nibabel?

larsoner · 2016-06-07T15:24:15Z

Avoiding a dependency IIRC

agramfort · 2016-06-07T20:35:23Z

yes and also when we needed this it was not released in nibabel. I agree that it should change to avoid to double maintenance

larsoner · 2016-06-20T13:55:59Z

mne/surface.py


 import numpy as np
 from scipy.sparse import coo_matrix, csr_matrix, eye as speye
+import nibabel as nib


nest this import we don't want a new hard dependency

jaeilepp · 2016-06-21T13:33:30Z

I noticed that make_flash_bem isn't tested on travis since it is classified as ultra_slow. Maybe it could be classified by as slow by not converting the images first.
So I was thinking of

precomputing flash5.mgz and adding it to the testing data (running time on my computer ca. 2 mins) or
precomputing flash5.mgz and flash5_reg.mgz and adding to them to testing data (running time some seconds, no slow tag).
Wdyt?

jaeilepp · 2016-06-21T13:34:35Z

mne/bem.py

        Path to SUBJECTS_DIR if it is not set in the environment.
+    flash_path : str | None
+        Path to the flash images. If None (default), the current folder is
+        used.


I ended up adding this to reduce the amount of magic with this function.

so not in mri/bem/flash anymore?

This was actually using the current folder before. Now we could default to the correct place, but is mri/bem/flash really the correct folder and not /mri/flash/parameter_maps? I can't find mri/bem/flash anywhere on my data.

indeed it should be: /mri/flash/parameter_maps or maybe just : /mri/flash/ see how it's done with sample

larsoner · 2016-06-21T13:36:21Z

Anything you can do to make a test not ultra_slow without compromising it too much is worthwhile. It would be nice to be able to do nosetests mne/tests/test_bem.py again without having it hang forever. So +1 for option (2) from me

agramfort · 2016-06-22T08:16:42Z

mne/commands/tests/test_commands.py



 @ultra_slow_test
-@requires_mne


agramfort · 2016-06-22T08:21:49Z

please set to MRG when comments are addressed. thx @jaeilepp was not that easy in the end ;)

larsoner · 2016-06-27T15:15:42Z

mne/utils.py

    'MNE_SKIP_TESTING_DATASET_TESTS',
    'MNE_STIM_CHANNEL',
    'MNE_USE_CUDA',
+    'MNE_TESTING',


We have others of the form MNE_SKIP_*, so maybe MNE_SKIP_FLASH_CALL or something would be better.

Although in this case, we want the default behavior when running nosetests mne/tests/test_bem.py to skip the FreeSurfer call, since it takes forever. Your current code will still make the call to FreeSurfer by default (although you have made AppVeyor and Travis skip it, developers will still be haunted by the slowness).

The calls to freesurfer here aren't actually that slow. It is the conversion of flash images that took a long time before. Now we have them precomputed.

jaeilepp · 2016-06-28T11:28:07Z

This is also ready for review.

larsoner · 2016-06-28T13:38:30Z

.travis.yml

        python -c 'import mne; mne.datasets.testing.data_path(verbose=True)';
        if [ "${DEPS}" == "full" ]; then
          export FREESURFER_HOME=$(python -c 'import mne; print(mne.datasets.testing.data_path())');
+          export MNE_SKIP_FLASH_CALL=1;


Is this done for speed, or because Travis and AppVeyor don't have the right command-line utilities to do it?

Ahh, you replied below:

Yeah, this is to avoid the freesurfer calls.

Can you add a comment somewhere saying that it's done only to avoid a command that isn't available on CIs?

agramfort · 2016-06-29T06:54:31Z

thanks @jaeilepp !

* 'master' of git://github.com/mne-tools/mne-python: (48 commits) FIX: Flake fix pep8 [MRG] FIX Topographic plotting for KIT-UMD data (mne-tools#3349) QUICKFIX: logging test Error message for simulate evoked. (mne-tools#3372) Warnings for big bems. (mne-tools#3374) Add test for baseline correction. Use apply_baseline in read_evokeds. Makes (None, 0) the default value baseline in apply_baseline. [FIX] Adds apply_baseline to Evoked fix component inds in ica tutorial (mne-tools#3379) FIX: Xdawn with shuffled epochs (mne-tools#3373) FIX consistency: Epochs.load_data() should always return self (mne-tools#3376) MRG: Build relevant examples (mne-tools#3365) [MRG] set vmin, vmax after smoothing in plot_epochs_image (mne-tools#3360) [FIX] Attempt to fix circle. fixing a bg plotting bug for topo plots Modified test and tutorial. [MRG] convert surface using python. (mne-tools#3273) [MRG] raw.plot_psd() with flat channel (mne-tools#3342) ...

jaeilepp reviewed May 31, 2016
View reviewed changes

larsoner reviewed Jun 20, 2016
View reviewed changes

jaeilepp reviewed Jun 21, 2016
View reviewed changes

agramfort reviewed Jun 22, 2016
View reviewed changes

mne/commands/tests/test_commands.py

@ultra_slow_test

@requires_mne

Copy link

Member

agramfort Jun 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah :)

jaeilepp force-pushed the convert-surface branch from edd4f12 to 4525e66 Compare June 22, 2016 11:25

larsoner reviewed Jun 27, 2016
View reviewed changes

jaeilepp force-pushed the convert-surface branch from 383d00c to 1723797 Compare June 28, 2016 13:34

jaeilepp added 17 commits June 28, 2016 15:35

[WIP] convert surface using python.

90ebbab

Remade the whole thing.

534cceb

Fixes.

f6e6beb

Read/write volume info

63a628d

Tests

1990994

Fixes.

c5f6db1

Volume info for tri surface.

e5cfb1e

Checks for nibabel.

9a14a56

Added flash_path to make_flash_bem.

2757b9e

Test for make_flash_bem.

17a0707

Fix to testing.

a42e1c7

Fix.

8931e10

Better fix.

d891b9f

Fix doc.

443160f

Testing.

7d65468

Fixes.

c4f07ae

Address comments. Updated dataset.

1723797

larsoner reviewed Jun 28, 2016
View reviewed changes

Address comments.

0196b28

agramfort merged commit 6be10db into mne-tools:master Jun 29, 2016

Uh oh!

[MRG] convert surface using python. #3273

[MRG] convert surface using python. #3273

Uh oh!

Conversation

jaeilepp commented May 31, 2016 • 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!

larsoner May 31, 2016 via email

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 May 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 77.80%

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!

agramfort commented May 31, 2016

Uh oh!

jaeilepp commented Jun 1, 2016

Uh oh!

jaeilepp commented Jun 2, 2016

Uh oh!

agramfort commented Jun 2, 2016 via email

Uh oh!

larsoner commented Jun 2, 2016 via email

Uh oh!

jaeilepp commented Jun 7, 2016

Uh oh!

agramfort commented Jun 7, 2016 via email

Uh oh!

jaeilepp commented Jun 7, 2016

Uh oh!

larsoner commented Jun 7, 2016

Uh oh!

agramfort commented Jun 7, 2016 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeilepp commented Jun 21, 2016

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 Jun 22, 2016 via email

Choose a reason for hiding this comment

Uh oh!

larsoner commented Jun 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeilepp commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeilepp commented May 31, 2016 •

edited

Loading

codecov-io commented May 31, 2016 •

edited

Loading