[MRG+1] kit2fiff support for .elp and .hsp files #3320

christianbrodbeck · 2016-06-18T12:28:27Z

Replacing #1001, adding support for files exported by the head shape digitizer used at UMD (and in the past at NYU NY).

For our KIT purposes we know that ELP/HSP are in [m] while TXT files are in [mm]. So I figured we could make things simpler by modifying _read_dig_points (https://github.com/mne-tools/mne-python/compare/master...christianbrodbeck:elp-hsp?expand=1#diff-74542bee8ed1be44ba6e68f0458010d8L358) such that it always return coordinates in [m](i.e. multiply coordinates from *.txt files with 0.001).

The mne.channels.read_dig_montage function also uses _read_dig_points, and in mne.channels.read_dig_montage (unlike the kit2fiff importers) the user specifies the unit the files are in, and the default is "mm". This leads to an ambiguous case where the users leaves unit="mm" but specifies *.hsp and *.elp files which we know (?) are in [m]. What should be the default behavior here?

just assume the user knows what they are doing and act as if they are in mm?
Ignore the value of unit for *.hsp and *.elp, assuming they are always in m?
Change the function signature default to unit=None and use mm for text and m for hsp/elp as long as unit is None?

We could also add a check that raises an error for unrealistic head diameters (>1m or <1cm) that way we should always catch erraneous units.

larsoner · 2016-06-18T15:44:49Z

+1 for unit='auto' mode

codecov-io · 2016-06-21T22:09:08Z

Current coverage is 86.67%

Merging #3320 into master will increase coverage by 0.02%

@@             master      #3320   diff @@
==========================================
  Files           337        337          
  Lines         58126      58176    +50   
  Methods           0          0          
  Messages          0          0          
  Branches       8862       8868     +6   
==========================================
+ Hits          50364      50424    +60   
+ Misses         5081       5072     -9   
+ Partials       2681       2680     -1

Powered by Codecov. Last updated by 2921f56...72eb10a

agramfort · 2016-06-22T08:03:52Z

mne/io/meas_info.py

+        Array of dig points in [m].
    """
-    dig_points = np.loadtxt(fname, comments=comments, ndmin=2)
+    if unit not in('auto', 'm', 'mm', 'cm'):


space after in

agramfort · 2016-06-22T08:05:40Z

is there any API change in this PR?

christianbrodbeck · 2016-06-22T12:46:08Z

mne/channels/montage.py

+        Unit of the digitizer files (hsp and elp). If not 'm', coordinates will
+        be rescaled to 'm'. Default is 'auto', which assumes 'm' for *.hsp and
+        *.elp files and 'mm' for *.txt files, corresponding to the known
+        Polhemus export formats.


Thanks @agramfort ! This here would be the API change (default to unit="auto" instead of unit="mm"). It does not change the behavior for previously possible argument combination because "auto" amounts to "mm" for *.txt files and arrays.

Currently (as well as before this PR) hsp and elp provided as arrays also get rescaled, I wonder if this could be confusing... if you agree I could add a check that raises an error or at least a warning if the head has a strange size (<10 mm or >1 m)

ok so there is no public API change. No code is going to break right?

Yes, no code using public API will break.

christianbrodbeck · 2016-06-22T14:50:27Z

mne/channels/montage.py

+            else:
+                raise TypeError('HPI file with extension *%s is not '
+                                'supported. Only *.txt, *.sqd and *.mrk are '
+                                'supported.' % ext)


Would the proper exception here be IOError?

I would use ValueError

Hm, should I change it or leave it for the sake of not changing API?

I doubt some catches specific TypeError. I would use ValueError

agramfort · 2016-06-23T07:55:14Z

KIT users any objections?

cc @kingjr @teonbrooks

christianbrodbeck · 2016-06-23T11:54:39Z

Rebased to add whats_new

agramfort · 2016-06-23T15:39:01Z

no objection to merge but I'd really like to have one of our KIT users to also test ....

kingjr · 2016-06-24T14:28:06Z

no objection, @teonbrooks?

kingjr · 2016-06-26T10:51:03Z

(ping me in two weeks if it's not merged, I'll be back in the lab)

teonbrooks · 2016-06-28T02:00:22Z

sorry for the delay, i'm a bit backlogged on things. if it's not a rush, I will get to it soonish. if not, someone else can review in my stead.

christianbrodbeck · 2016-06-28T02:03:20Z

No rush, I know you're traveling

dengemann · 2016-06-28T20:47:02Z

Definitely stuff for the kit people. Make sure also here that you few people who use this code are happy.

teonbrooks · 2016-06-29T09:26:10Z

mne/gui/help/kit2fiff.json

@@ -1,7 +1,10 @@
 {
+  "hsp_file": "*.hsp or *.txt file containing digitized head shape points",
+  "fid_file": "*.elp or *.txt file containing 8 digitized fiducial points (\"electrode laser points\")",


should we be calling this fid_file? only the first three points are typically referred to as fiducials. wouldn't elp_file be a better name?

That's just how it's called in the traits gui already... :) technically aren't all 8 points fiducial points?

according to wikipedia, yes :) https://en.wikipedia.org/wiki/Fiducial_marker#Medical_imaging

However looking at that description again, what does ELP really stand for? "Electrode Location Point"?

teonbrooks · 2016-06-29T09:40:24Z

other than those concerns, LGTM

jaeilepp · 2016-07-01T12:49:18Z

mne/channels/montage.py

    .. versionadded:: 0.9.0
    """
-    if not isinstance(unit, string_types) or unit not in('m', 'mm', 'cm'):
-        raise ValueError('unit must be "m", "mm", or "cm"')


I would keep the sanity check. If I understand correctly, with this solution, data will just not be scaled if someone passes something erroneus (like "CM").

Thanks for catching that! See 76ab987

christianbrodbeck · 2016-07-11T20:23:42Z

ping @teonbrooks @kingjr are there any concerns left here?

kingjr · 2016-07-11T20:37:16Z

Could you make a snippet so that I can adapt and test it onto my data?

On 11 July 2016 at 16:23, Christian Brodbeck notifications@github.com
wrote:

ping @teonbrooks https://github.com/teonbrooks @kingjr
https://github.com/kingjr are there any concerns left here?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3320 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AEp7DMRH0XwIuZpQM6sZk3KkhZ6LbQufks5qUqZQgaJpZM4I478m
.

christianbrodbeck · 2016-07-11T20:54:46Z

@kingjr the GUI will be useful to verify that coordinates are read correctly, and you can use the testing data

import mne
import os

root = os.path.dirname(mne.io.kit.__file__)
data_dir = root + '/tests/data/'
hsp = data_dir + 'test.hsp'
elp = data_dir + 'test.elp'
mrk = data_dir + 'test_mrk.sqd'
sqd = data_dir + 'test.sqd'

raw = mne.io.read_raw_kit(sqd, mrk, elp, hsp)
mne.channels.read_dig_montage(hsp, mrk, elp, ['nasion', 'lpa', 'rpa'] + [''] * 5)

teonbrooks · 2016-07-13T20:01:16Z

that's it for me. +1

agramfort · 2016-07-14T08:11:08Z

@christianbrodbeck you need to rebaase

…auto’

christianbrodbeck · 2016-07-14T14:36:15Z

Rebased and fixed the exception

larsoner · 2016-07-18T19:45:53Z

@kingjr do you have time to take one last look?

kingjr · 2016-07-19T18:41:55Z

I didn't manage to run the snippet because I don't have the kit sample data (how do I download that?)

But I think if @teonbrooks is happy is probably good.

christianbrodbeck · 2016-07-19T18:42:56Z

@kingjr this is the testing data in the snippet, it should be part of mne after this PR

kingjr · 2016-07-19T18:45:51Z

mne/io/kit/tests/test_coreg.py

+    pts_2 = read_mrk(fname)
+    assert_array_equal(pts_2, pts, "pickle mrk")
+    with open(fname, 'wb') as fid:
+        pickle.dump({}, fid)


explicit dict()

Why is dict() better than {}? Just curious.

For Matlab converts? :)

I'm not sure where the convention comes from. I think it may because it's indeed more explicit to those who don't speak python, but I'm not sure.

kingjr · 2016-07-19T18:47:43Z

LGTM

christianbrodbeck · 2016-07-26T17:29:59Z

Addressed the remaining issues. I don't know what's wrong with CI, it's indicating it can't find some SUBJECTS_DIR, which does not seem related to the changes in the last commit.

larsoner · 2016-07-27T02:55:38Z

mne/io/kit/tests/test_kit.py

+
+def test_hsp_elp():
+    """Test KIT usage of *.elp and *.hsp files against *.txt files
+    """


FYI our converged convention is now to put these on the same line like """Test ... """

Also in #3463

larsoner · 2016-07-27T02:57:13Z

I agree the CI is unrelated, shouldn't happen on master build hopefully. Last tweaks can be made later. Thanks @christianbrodbeck

massich · 2019-02-21T14:55:45Z

cross reference #5836 #5975

agramfort reviewed Jun 22, 2016
View reviewed changes

christianbrodbeck reviewed Jun 22, 2016
View reviewed changes

christianbrodbeck force-pushed the elp-hsp branch from 6fac539 to 2da0f75 Compare June 22, 2016 14:43

christianbrodbeck reviewed Jun 22, 2016
View reviewed changes

christianbrodbeck force-pushed the elp-hsp branch from 2da0f75 to 96a296f Compare June 22, 2016 15:18

christianbrodbeck changed the title ~~[WIP] kit2fiff support for *.elp and *.hsp files~~ [MRG] kit2fiff support for *.elp and *.hsp files Jun 22, 2016

christianbrodbeck force-pushed the elp-hsp branch from 96a296f to b914951 Compare June 23, 2016 11:53

teonbrooks reviewed Jun 29, 2016
View reviewed changes

christianbrodbeck force-pushed the elp-hsp branch from e3fb301 to a3f8e3c Compare June 29, 2016 17:12

jaeilepp reviewed Jul 1, 2016
View reviewed changes

christianbrodbeck force-pushed the elp-hsp branch from 76ab987 to 37eabdb Compare July 2, 2016 00:24

christianbrodbeck and others added 11 commits July 14, 2016 09:15

API read_dig_montage: add support for *.elp and *.hsp, default unit=‘…

cfb6084

…auto’

PEP8

948fd39

STY

46f2319

DOC

e63ef17

TEST kit/coreg marker I/O

786080a

TEST read_dig_montage

13b90f6

DOC: whats_new

f33f8f2

TEST transformed ELP points

54bbc61

DOC

ca840f9

FIX read_dig_montage: check unit is valid

9e91b84

FIX Exception type

72eb10a

christianbrodbeck force-pushed the elp-hsp branch from 37eabdb to 72eb10a Compare July 14, 2016 13:19

larsoner changed the title ~~[MRG] kit2fiff support for *.elp and *.hsp files~~ [MRG+1] kit2fiff support for *.elp and *.hsp files Jul 18, 2016

kingjr reviewed Jul 19, 2016
View reviewed changes

STY

531564e

larsoner reviewed Jul 27, 2016
View reviewed changes

larsoner merged commit 727a871 into mne-tools:master Jul 27, 2016

christianbrodbeck deleted the elp-hsp branch July 27, 2016 14:56

massich added the Digitization label Feb 21, 2019

Uh oh!

[MRG+1] kit2fiff support for *.elp and *.hsp files #3320

[MRG+1] kit2fiff support for *.elp and *.hsp files #3320

Uh oh!

Conversation

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

Uh oh!

Uh oh!

larsoner commented Jun 18, 2016

Uh oh!

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

Uh oh!

Current coverage is 86.67%

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!

agramfort Jun 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jun 23, 2016

Uh oh!

christianbrodbeck commented Jun 23, 2016

Uh oh!

agramfort commented Jun 23, 2016

Uh oh!

kingjr commented Jun 24, 2016

Uh oh!

kingjr commented Jun 26, 2016

Uh oh!

teonbrooks commented Jun 28, 2016

Uh oh!

christianbrodbeck commented Jun 28, 2016

Uh oh!

dengemann commented Jun 28, 2016

Uh oh!

teonbrooks Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

teonbrooks commented Jun 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christianbrodbeck commented Jul 11, 2016

Uh oh!

kingjr commented Jul 11, 2016

Uh oh!

christianbrodbeck commented Jul 11, 2016

Uh oh!

teonbrooks commented Jul 13, 2016

Uh oh!

agramfort commented Jul 14, 2016

Uh oh!

christianbrodbeck commented Jul 14, 2016

Uh oh!

larsoner commented Jul 18, 2016

Uh oh!

kingjr commented Jul 19, 2016

[MRG+1] kit2fiff support for .elp and .hsp files #3320

[MRG+1] kit2fiff support for .elp and .hsp files #3320

christianbrodbeck commented Jun 18, 2016 •

edited

Loading

codecov-io commented Jun 21, 2016 •

edited

Loading

teonbrooks Jun 29, 2016 •

edited

Loading