nibabel.freesurfer.write_annot successfully writes files that cannot be read by freesurfer

[mwaskom]

I will edit this issue when I can come up with a good example of this separate from my data, but the headline is that the write_annot function can write (without erroring) a file that Freesurfer is unable to read.

In the current case, nib.freesurfer.read_annot is also unable to read the written file. However, I have also experienced that nib.freesurfer.read_annot will read the file but it will be incorrect.

Edit: and I have now written an annotation that Freesurfer can read but nibabel cannot.

[effigies]

@mwaskom Do you have examples of each type of file that you can share?

[effigies]

@mwaskom Just a bump to see if there's any data you can post.

[mwaskom]

Closing as I don't have time to fiddle around with annotations (almost certainly the murkiest Freesurfer file format) right now.

[effigies]

Alright. Please reopen if you do get time.

[rnemec-ng]

I have a fix for this one but not much time to create a PR.
The fix is in io.py function write_annot, change

        # maxstruc
        write(np.max(labels) + 1)

to

        # maxstruc
        write(max(np.max(labels) + 1, ctab.shape[0]))

That will write the annot file in a way that can be read by read_annot, freeview and others. At least with the files I have been using...

[pauldmccarthy]

Hey @rnemec-ng, what version of nibabel have you been using? This shoul hopefully have been resolved as of 2.3.1:

• Nibabel 2.3 freesurfer.read_annot() breaks backwards compatibility #649
• FIX: Annotation labels are packed RGB, not RGBA #666
• FIX: Label construction in read_annotation.m  freesurfer/freesurfer#541

[rnemec-ng]

@pauldmccarthy - from my requirements.txt:

• nibabel==2.3.1

I believe the problem is that the annot file docs say the maxstruc field (that I patched) is "Max structure index (could be more than actual entries, since some may be omitted)" but in reality it can also be less than the number of entries (field num_entries) which seems to not be handled properly in multiple places (e.g. read_annot) when the maximum labels value is less than the number of entries.

In theory, the read_annot should also be fixed to not use the maxstruc for allocation, and just the num_entries. I have that change, too, if anybody is interested.

I still prefer to fix the write_annot since this fix allows my files to be FINALLY (!) opened by freeview.

[effigies]

@rnemec-ng Any contribution you can make would be welcome. If you could submit your patches as a minimal PR, then someone with a bit of time on their hands could try to put together tests and polish it up.

For writing those tests, it would be useful to have a file that fails to be read correctly by read_annot, if you have any, or code to generate an invalid file that will be fixed by your patches.

I appreciate that you said you don't have much time, so whatever you're able to supply would be helpful.

[rnemec-ng]

Not much time, not much shmime... 😉 ... whatever.
Here is a simple reproduction:

def reproBug():
    nlabels = 3
    names = ['label {}'.format(l) for l in range(1, nlabels + 1)]
    labels = [1, 1, 1]
    labels = np.array(labels, dtype=np.int32)
    rgba = np.array(np.random.randint(0, 255, (nlabels, 4)), dtype=np.int32)
    annot_path = 'c.annot'

    nib.freesurfer.io.write_annot(annot_path, labels, rgba, names)
    nib.freesurfer.io.read_annot(annot_path)

Fails with current code (and freeview), succeeds with my tiny patch.
Maybe the above is 90% of the test code to add, I guess...

[effigies]

@rnemec-ng I'm applying the fix you propose in #763. Looking at this code, I'm not very comfortable with how we do this, as I could create a label with ID 2^31-1 and cause read_annot to chew up a ton of memory pointlessly. That said, that's already true, and your fix doesn't make it worse.

Anyway, there's presumably a read issue here, as well, because the read code will simply write to index 1 three times.

[rnemec-ng]

Thank you Chris for looking into this issue. I'm sure you know much more than I do about the code and whether "my fix" is the right one or not. I'll be happy with any improvements, as long as they allow reading files by both nibabel as well as freesurfer.

[effigies]

Well, I added a round-trip check, and at least on my machine it looks like we read the same things we write, so I guess my worry was misplaced. Do you want to try out #763 and verify it works for your files?

[rnemec-ng]

Not much time to test it (other than the test code you already put in place). At least, I added approval to the PR (but didn't merge). Hope this helps

[effigies]

Thanks. We can open a new issue if it turns out something else needs doing.