WIP : ugly hack to fix the read_annot when parcellation has missing values #194

agramfort · 2013-08-23T13:55:32Z

the issue comes with annot that have missing values ie that don't cover the entire cortex. One issue I don't know how to address it that there is no np.nan for integers and we index at 0 for indexing with colortab output.

any hint welcome

addresses #189

cc @mwaskom

matthew-brett · 2013-08-23T15:14:35Z

Thanks for this. Can you think of a test?

matthew-brett · 2013-08-23T15:22:42Z

Can the freesurfer guys give any comment on this one?

mwaskom · 2013-08-23T15:48:20Z

I guess this looks reasonable. I can't think of a clean way to handle it :/

mwaskom · 2013-08-23T15:48:42Z

Is it worth raising a warning here?

mwaskom · 2013-08-23T16:41:57Z

Also maybe add a boolean argument to the function to "fix" incomplete annotations (a default value of True is fine)? I could think of non-visualization cases where people will expect the output of the function to be exactly what was saved into the file.

agramfort · 2013-08-23T20:18:01Z

I am -1 for the boolean argument. I've just updated the docstring that was
not up to date...

All the lines are covered by tests but It's hard to add a test as the
freesurfer recon only outputs full annots.
We would need to add sample files in the repo, which we don't do for this
part of the code.

I think we can live with it :)

On Fri, Aug 23, 2013 at 6:41 PM, Michael Waskom notifications@github.comwrote:

Also maybe add a boolean argument to the function to "fix" incomplete
annotations (a default value of True is fine)? I could think of
non-visualization cases where people will expect the output of the function
to be exactly what was saved into the file.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/194#issuecomment-23175637
.

matthew-brett · 2013-08-23T20:46:24Z

How hard would it be to create a tiny sample file for testing?

agramfort · 2013-08-24T13:16:45Z

the freesurfer module has no data for testing and the default freesurfer
does not provide such files. I can commit the file provided
'left_mc-z.negative.sig.ocn.annot" it's 82KB. If so should I put it in
freesurfer/tests/data ?

matthew-brett · 2013-08-24T19:11:31Z

How about gzipping it down to 20K? We do that for the DICOM files, for example.

We can't easily write annot files I guess?

agramfort · 2013-08-25T11:51:11Z

How about gzipping it down to 20K? We do that for the DICOM files, for
example.

does it mean my annot reader need to support .gz files? Can you give a
snippet.

We can't easily write annot files I guess?

Indeed. We don't have writers.

matthew-brett · 2013-08-25T19:28:04Z

Well - you would need to support file objects...

Maybe that is a worthwhile change to read_annot? As in (top of function):

from nibabel.openers import Opener
with Opener(file_like) as fobj:

That way you get file-like objects as input for free...

agramfort · 2013-08-26T07:55:09Z

ok I'll give it a try asap

agramfort · 2013-08-27T20:15:10Z

should be good to go after some clarification from doug on the freesurfer ML

there was a real bug with orig_ids=False :-/

luckily pysurfer uses orig_ids=True...

I am now wondering if we should not deprecate orig_ids=False ...

cc @mwaskom

agramfort · 2013-08-27T20:15:34Z

btw I forced push after squashing my commits

matthew-brett · 2013-08-27T20:37:56Z

I don't understand the issues here :(

Are you now testing the orig_ids=False code path with 0s?

agramfort · 2013-08-27T20:52:03Z

It turns out that the annot provided by freesurfer also has 0 values. Is was just unnoticed before ...

…g_ids=False. Vertices with 0 label were randomly merged with the first entry in the colortable

agramfort · 2013-08-27T21:24:14Z

see my last change in test (squashed)

matthew-brett · 2013-08-27T21:30:39Z

Please forgive me - I must be confused. Am I right in thinking there are no extra tests for the orid_ids=False code path in this PR?

agramfort · 2013-08-27T22:13:22Z

Try the test with master. It should fail

matthew-brett · 2013-08-27T22:20:52Z

It does. I understand why now; you are testing the labels for orig_ids=False against the labels for orig_ids=True hence you are testing orig_ids=False.

Are you happy with this fix? What about deprecating orig_ids=False? It is OK that this is the default?

mwaskom · 2013-08-28T02:35:41Z

Sorry as I'm in New York on vacation I haven't had time to fully wrap my
head around this. But I think I remain in favor of orig_ids=False. The
Freesurfer annot format is a little weird in that the values given to each
region are related to the color in the lookup table (I believe the id is R

(G * 256) + (B * 256 ^ 2)). So you end up with these very large numbers
that have no obvious correspondence to each other or anything else. The LUT
is in the annotation file anyway, making the information redundant. I much
prefer the obvious data representation of sequential numbers.

Is there a reason we can't make the undefined region NaN?

On Tue, Aug 27, 2013 at 6:20 PM, Matthew Brett notifications@github.comwrote:

It does. I understand why now; you are testing the labels for
orig_ids=False against the labels for orig_ids=True hence you are testing
orig_ids=False.

Are you happy with this fix? What about deprecating orig_ids=False? It is
OK that this is the default?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/194#issuecomment-23376012
.

agramfort · 2013-08-28T07:13:48Z

Sorry as I'm in New York on vacation I haven't had time to fully wrap my
head around this. But I think I remain in favor of orig_ids=False. The
Freesurfer annot format is a little weird in that the values given to each
region are related to the color in the lookup table (I believe the id is R

(G * 256) + (B * 256 ^ 2)).

indeed

So you end up with these very large numbers
that have no obvious correspondence to each other or anything else. The LUT
is in the annotation file anyway, making the information redundant. I much
prefer the obvious data representation of sequential numbers.

fair enough. But we have now -1 in label which is not a valid index.
I can live with it though.

Is there a reason we can't make the undefined region NaN?

yes. NaN cannot be in an integer array such as label.

mwaskom · 2013-08-28T18:46:14Z

Sorry again, i'm probably missing something obvious, but do we do something
such that label has to be int typed? Would round floats not work?

On Wed, Aug 28, 2013 at 3:13 AM, Alexandre Gramfort <
notifications@github.com> wrote:

Sorry as I'm in New York on vacation I haven't had time to fully wrap my
head around this. But I think I remain in favor of orig_ids=False. The
Freesurfer annot format is a little weird in that the values given to
each
region are related to the color in the lookup table (I believe the id is
R

(G * 256) + (B * 256 ^ 2)).

indeed

So you end up with these very large numbers
that have no obvious correspondence to each other or anything else. The
LUT
is in the annotation file anyway, making the information redundant. I
much
prefer the obvious data representation of sequential numbers.

fair enough. But we have now -1 in label which is not a valid index.
I can live with it though.

Is there a reason we can't make the undefined region NaN?

yes. NaN cannot be in an integer array such as label.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/194#issuecomment-23395467
.

agramfort · 2013-08-28T21:19:55Z

Sorry again, i'm probably missing something obvious, but do we do something
such that label has to be int typed? Would round floats not work?

it could but it's kind of ugly to store indices as floats

matthew-brett · 2013-08-29T00:19:05Z

Sorry - closed by accident.

agramfort · 2013-08-29T06:39:25Z

so? what's our status?

mwaskom · 2013-08-29T14:21:50Z

OK I think your point about float indices being janky is correct. I guess
it's not really our fault that the format spec is a little weird, and your
-1 solution seems sound.

So I am +1 on -1 :)

On Thu, Aug 29, 2013 at 2:39 AM, Alexandre Gramfort <
notifications@github.com> wrote:

so? what's our status?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/194#issuecomment-23470005
.

agramfort · 2013-08-29T14:23:56Z

matthew feel free to merge if you approve

is there a place to mention bug fixes? release notes?

matthew-brett · 2013-08-29T19:21:20Z

For bug fix mentions - can you add something to the Changelog about this? I make the release notes from the Changelog file.

agramfort · 2013-08-29T20:02:52Z

I don't see any current devel section in changelog. can you take care of it?

note:

bug fix in freesurfer.read_annot with orig_ids=False when annot contains
vertices with no label.

thanks

MRG : ugly hack to fix the read_annot when parcellation has missing values The issue comes with annot that have missing values ie that don't cover the entire cortex.

sanadeem · 2013-09-26T02:52:27Z

Hi

With this hack, I am getting a type error at the following line:

labels[mask] = ord[np.searchsorted(ctab[ord, -1], labels[mask])]

TypeError: array cannot be safely cast to required type

Thanks

matthew-brett · 2013-09-26T06:37:43Z

Can you point us to an example file with which we can replicate the problem? Thanks for any help in debugging this.

agramfort · 2013-09-26T07:00:55Z

hi,

do you get this with any annot? what numpy version are you using? if it's
for one annot can you share it?

A

On Thu, Sep 26, 2013 at 4:52 AM, sanadeem notifications@github.com wrote:

Hi

With this hack, I am getting a type error at the following line:

labels[mask] = ord[np.searchsorted(ctab[ord, -1], labels[mask])]

TypeError: array cannot be safely cast to required type

Thanks

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/194#issuecomment-25140577
.

agramfort · 2013-09-26T16:07:02Z

weird I do:

In [8]: import nibabel as nib
In [9]: import numpy as np
In [10]: np.version
Out[10]: '1.6.1'
In [11]: nib.freesurfer.read_annot('lh.aparc.annot')

and everything works fine. Can you debug in ipython to figure out the type
issue?

sanadeem · 2013-09-26T19:13:18Z

Apparently the error doesn't occur with numpy 1.7.1 and only occurs with the assignment operation of
labels[mask] = ord[np.searchso....] under numpy version 1.6.1. Once I updated to numpy 1.7.1, it worked fine.

agramfort · 2013-09-27T08:08:53Z

good to know but we cannot forget this anyway. Could you find out what the
problem is with your numpy 1.6.1?

thanks

matthew-brett · 2013-09-27T08:31:54Z

Interesting - in a numpy 1.6.0 virtualenv, I get this:

(np-1.6.0)[mb312@tom ~/dev_trees/nibabel (main-master)]$ python -c 'import nibabel.freesurfer.io as nfi; nfi.read_annot("lh.aparc.annot")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "nibabel/freesurfer/io.py", line 185, in read_annot
    data = np.fromfile(fobj, dt, vnum * 2).reshape(vnum, 2)
ValueError: total size of new array must be unchanged

On my desktop machine running numpy 1.6.2 python 2.7, the same command gives:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "nibabel/freesurfer/io.py", line 185, in read_annot
    data = np.fromfile(fobj, dt, vnum * 2).reshape(vnum, 2)
MemoryError

sanadeem · 2013-09-27T11:27:40Z

Interesting. On another machine with a clean installation of nibabel, with numpy 1.6.2 and python 2.7,
I get a different error as that mentioned by Matthew. Let me look into this, further.

sanadeem · 2013-09-27T15:12:05Z

I found another weird thing. When everything is working fine, the last conditional statement doesn't seem to go through when orig_ids=False. I have verified this on three different machines.

agramfort · 2013-09-27T15:36:44Z

I am sorry I cannot reproduce these failures :(

MRG : ugly hack to fix the read_annot when parcellation has missing values The issue comes with annot that have missing values ie that don't cover the entire cortex.

agramfort mentioned this pull request Aug 23, 2013

reading freesurfer annotation files with nibabel.freesurfer.read_annot() returns faulty vertex indices #189

Closed

FIX : fix the read_annot when parcellation has missing values and ori…

13b17af

…g_ids=False. Vertices with 0 label were randomly merged with the first entry in the colortable

matthew-brett closed this Aug 29, 2013

matthew-brett reopened this Aug 29, 2013

matthew-brett merged commit fd365c7 into nipy:master Aug 30, 2013

WIP : ugly hack to fix the read_annot when parcellation has missing values #194

WIP : ugly hack to fix the read_annot when parcellation has missing values #194

Conversation

agramfort commented Aug 23, 2013

matthew-brett commented Aug 23, 2013

matthew-brett commented Aug 23, 2013

mwaskom commented Aug 23, 2013

mwaskom commented Aug 23, 2013

mwaskom commented Aug 23, 2013

agramfort commented Aug 23, 2013

matthew-brett commented Aug 23, 2013

agramfort commented Aug 24, 2013

matthew-brett commented Aug 24, 2013

agramfort commented Aug 25, 2013

matthew-brett commented Aug 25, 2013

agramfort commented Aug 26, 2013

agramfort commented Aug 27, 2013

agramfort commented Aug 27, 2013

matthew-brett commented Aug 27, 2013

agramfort commented Aug 27, 2013

agramfort commented Aug 27, 2013

matthew-brett commented Aug 27, 2013

agramfort commented Aug 27, 2013

matthew-brett commented Aug 27, 2013

mwaskom commented Aug 28, 2013

agramfort commented Aug 28, 2013

mwaskom commented Aug 28, 2013

agramfort commented Aug 28, 2013

matthew-brett commented Aug 29, 2013

agramfort commented Aug 29, 2013

mwaskom commented Aug 29, 2013

agramfort commented Aug 29, 2013

matthew-brett commented Aug 29, 2013

agramfort commented Aug 29, 2013

sanadeem commented Sep 26, 2013

matthew-brett commented Sep 26, 2013

agramfort commented Sep 26, 2013

agramfort commented Sep 26, 2013

sanadeem commented Sep 26, 2013

agramfort commented Sep 27, 2013

matthew-brett commented Sep 27, 2013

sanadeem commented Sep 27, 2013

sanadeem commented Sep 27, 2013

agramfort commented Sep 27, 2013