[MRG+2] Surf metadata #460

jaeilepp · 2016-06-10T09:10:23Z

Takeover from #458
cc: @agramfort @effigies
closes #456
I could not figure out the 'tail' of extra information, so I suggest we just dump it for the time being.

coveralls · 2016-06-10T09:23:13Z

Coverage decreased (-0.003%) to 96.095% when pulling 2e01d49 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

codecov-io · 2016-06-10T09:52:18Z

Current coverage is 94.11%

Merging #460 into master will increase coverage by <.01%

@@             master       #460   diff @@
==========================================
  Files           160        160          
  Lines         21090      21162    +72   
  Methods           0          0          
  Messages          0          0          
  Branches       2244       2264    +20   
==========================================
+ Hits          19849      19917    +68   
  Misses          823        823          
- Partials        418        422     +4

Powered by Codecov. Last updated by 9959c74...07bd848

coveralls · 2016-06-10T10:04:24Z

Coverage decreased (-0.003%) to 96.095% when pulling b5f1849 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

larsoner · 2016-06-10T14:37:10Z

@jaeilepp I didn't know you took this over. @agramfort suggested I do it, so I made a commit to #458. It looks like your code is a bit cleaner, but maybe you could make use of the test modifications I made.

agramfort · 2016-06-10T15:07:24Z

travis is not happy

effigies · 2016-06-10T16:33:30Z

nibabel/freesurfer/io.py

+def _read_volume_info(fobj):
+    volume_info = OrderedDict()
+    head = np.fromfile(fobj, '>i4', 3)
+    if any(head != [2, 0, 20]):


I think this will always fail, as the RHS is a list, not a numpy array. Might do if head not in (0, 2, 20):

Oh, I see, this is trying to check list equality. In that case, you'd want if not np.array_equal(head, [2, 0, 20]), but consider also these files, which are just 20.

So maybe:

if head != 20 and not np.array_equal(head, [2, 0, 20]):

jaeilepp · 2016-06-13T07:50:14Z

nibabel/freesurfer/io.py

+                        key, val[0], val[1], val[2]).encode('utf-8'))
+                else:
+                    fobj.write('{0} = {1:.4f} {2:.4f} {3:.4f}\n'.format(
+                        key.ljust(6), val[0], val[1], val[2]).encode('utf-8'))


Note that currently this expects the data be in the same format that it was read. So no arbitrary extra data is allowed.

jaeilepp · 2016-06-13T07:50:36Z

Comments addressed.

coveralls · 2016-06-13T08:01:03Z

Coverage increased (+0.01%) to 96.107% when pulling 880152a on jaeilepp:surf-metadata into 9959c74 on nipy:master.

agramfort · 2016-06-13T12:41:06Z

LGTM

jaeilepp · 2016-06-13T13:26:24Z

I suppose Appveyor failures are unrelated?

agramfort · 2016-06-13T13:28:44Z

yes it seems a conda pb

matthew-brett · 2016-06-13T17:07:25Z

nibabel/freesurfer/io.py

+        if pair[0].strip() != key or len(pair) != 2:
+            raise IOError('Error parsing volume info.')
+        if key in ('valid', 'filename'):
+            volume_info[key] = pair[1].strip()


Is there no way that the filename could have trailing whitespace, such as carriage returns? Worth stripping the pair values before starting?

The values are in effect stripped. See the conditional above.

larsoner · 2016-06-14T15:30:28Z

nibabel/freesurfer/io.py

+            'xras' : array of float, shape (3,)
+            'yras' : array of float, shape (3,)
+            'zras' : array of float, shape (3,)
+            'cras' : array of float, shape (3,)


I suspect this will look bad when rendered. Have you checked? This is more likely to work:

Valid keys: * ... * ...

jaeilepp · 2016-06-15T07:47:17Z

I just realized that there are some numerical errors because we read the values to arrays, but I suppose it's not a problem. For instance reading:

xras=[ -1.00000000e+00   8.56816911e-08   1.49011647e-08]
yras=[  1.15484021e-07   1.57160827e-08  -1.00000000e+00]
zras=[ -1.91852465e-07   1.00000000e+00   6.40284092e-09]
cras=[ -5.27361298   9.03908539 -27.28796387]

becomes:

xras=[-1.  0.  0.]
yras=[ 0.  0. -1.]
zras=[-0.  1.  0.]
cras=[ -5.273613   9.039085 -27.287964]

when written back to a file.

coveralls · 2016-06-15T07:48:16Z

Coverage increased (+0.003%) to 96.1% when pulling 98930e9 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

coveralls · 2016-06-15T08:39:12Z

Coverage increased (+0.003%) to 96.1% when pulling e73e3d2 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

coveralls · 2016-06-15T09:50:49Z

Coverage increased (+0.003%) to 96.1% when pulling bdacad1 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

larsoner · 2016-06-15T11:17:43Z

I just realized that there are some numerical errors because we read the values to arrays

Those values can be stored using double floating point just fine. Maybe you're writing them out in a bad format?

larsoner · 2016-06-15T11:21:37Z

nibabel/freesurfer/io.py

+                key, val[0], val[1], val[2]).encode('utf-8'))
+        else:
+            val = volume_info[key]
+            strings.append('{0} = {1:f} {2:f} {3:f}\n'.format(


This is the problem, try something like {1:0.9g} ... and you'll see it's written with higher precision.

Okay, I missed this comment. 0.10g seems to work.

jaeilepp · 2016-06-15T11:47:37Z

If I change to general format I get cras=[ -5.27361298 9.03908539 -27.28796387] changed to cras=[ -5.27361 9.03909 -27.288 ]. The others translate fine. I'm not sure if there's a format that works well for all values.
EDIT: 0.10g works.

coveralls · 2016-06-15T11:55:30Z

Coverage increased (+0.003%) to 96.1% when pulling 10e56f9 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

coveralls · 2016-06-15T12:33:05Z

Coverage increased (+0.01%) to 96.111% when pulling 07bd848 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

jaeilepp · 2016-06-15T13:01:44Z

Ready for review/merge.

larsoner · 2016-06-15T14:58:04Z

nibabel/freesurfer/tests/test_io.py


+        assert_equal(volume_info2['cras'], volume_info['cras'])


Now that you have improved the precision in writing, you can use something more complete like:

for key in ('xras', 'yras', 'zras', 'cras'): assert_allclose(volume_info2[key], volume_info[key], rtol=1e-7, atol=1e-30)

This would have failed under the %f writing but passes now with the %0.10g writing, right?

In theory yes, but I couldn't find a test file that would have this kind of precision in the end.

larsoner · 2016-06-15T15:01:31Z

Other than my last (valid) comment, LGTM

coveralls · 2016-06-16T06:25:43Z

Coverage increased (+0.01%) to 96.111% when pulling 45f6052 on jaeilepp:surf-metadata into 9959c74 on nipy:master.

jaeilepp · 2016-06-17T09:07:30Z

This is ready for review. Appveyor error seems unrelated.

agramfort · 2016-06-17T09:53:17Z

+1 for merge on my side

larsoner · 2016-06-17T12:38:14Z

+1 for merge from me too

matthew-brett · 2016-06-17T17:13:47Z

Thanks @jaeilepp for your patient work on this, and thanks too to y'all for the careful reviews.

MRG: Fix to surface reading with new quad. Follow up to PR #460 which implemented read of surface data. The data format is actually different when reading new quad files. Use a file from freesurfer that has this format (added in the nibabel-data submodules) to test.

effigies and others added 6 commits June 7, 2016 13:53

NF: Handle metadata in read/write_geometry

ddf9db1

add test + misc

63fd6c9

address comments

995dc75

address comments

c41faad

don't count warnings

89203bb

Reading of extra info.

2e01d49

Fix

b5f1849

larsoner mentioned this pull request Jun 10, 2016

Fs surf metadata #458

Closed

effigies reviewed Jun 10, 2016
View reviewed changes

Changed from strings to np.arrays. Allow head == 20.

880152a

jaeilepp reviewed Jun 13, 2016
View reviewed changes

matthew-brett reviewed Jun 13, 2016
View reviewed changes

larsoner reviewed Jun 14, 2016
View reviewed changes

_serialize_volume_info

98930e9

String to bytes.

e73e3d2

Fix.

bdacad1

larsoner reviewed Jun 15, 2016
View reviewed changes

General format.

10e56f9

Test coverage.

07bd848

larsoner reviewed Jun 15, 2016
View reviewed changes

Added a test.

45f6052

agramfort changed the title ~~Surf metadata~~ [MRG+1] Surf metadata Jun 17, 2016

larsoner changed the title ~~[MRG+1] Surf metadata~~ [MRG+2] Surf metadata Jun 17, 2016

matthew-brett merged commit f4a3e0a into nipy:master Jun 17, 2016

jaeilepp mentioned this pull request Jun 21, 2016

[MRG] Fix to surface reading with new quad. #461

Merged

[MRG+2] Surf metadata #460

[MRG+2] Surf metadata #460

Conversation

jaeilepp commented Jun 10, 2016

coveralls commented Jun 10, 2016

codecov-io commented Jun 10, 2016 • edited Loading

Current coverage is 94.11%

coveralls commented Jun 10, 2016

larsoner commented Jun 10, 2016

agramfort commented Jun 10, 2016

Choose a reason for hiding this comment

effigies Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeilepp commented Jun 13, 2016

coveralls commented Jun 13, 2016

agramfort commented Jun 13, 2016

jaeilepp commented Jun 13, 2016

agramfort commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeilepp commented Jun 15, 2016

coveralls commented Jun 15, 2016 • edited Loading

coveralls commented Jun 15, 2016 • edited Loading

coveralls commented Jun 15, 2016 • edited Loading

larsoner commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeilepp commented Jun 15, 2016 • edited Loading

coveralls commented Jun 15, 2016 • edited Loading

coveralls commented Jun 15, 2016 • edited Loading

jaeilepp commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner commented Jun 15, 2016

coveralls commented Jun 16, 2016 • edited Loading

jaeilepp commented Jun 17, 2016

agramfort commented Jun 17, 2016

larsoner commented Jun 17, 2016

matthew-brett commented Jun 17, 2016

codecov-io commented Jun 10, 2016 •

edited

Loading

effigies Jun 10, 2016 •

edited

Loading

coveralls commented Jun 15, 2016 •

edited

Loading

coveralls commented Jun 15, 2016 •

edited

Loading

coveralls commented Jun 15, 2016 •

edited

Loading

jaeilepp commented Jun 15, 2016 •

edited

Loading

coveralls commented Jun 15, 2016 •

edited

Loading

coveralls commented Jun 15, 2016 •

edited

Loading

coveralls commented Jun 16, 2016 •

edited

Loading