Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Downstream FreeSurfer surface I/O broken #699

Closed
larsoner opened this issue Aug 20, 2019 · 1 comment
Closed

BUG: Downstream FreeSurfer surface I/O broken #699

larsoner opened this issue Aug 20, 2019 · 1 comment

Comments

@larsoner
Copy link
Contributor

A newline appears to have been removed from MRISwriteTriangularSurface at some point, and this breaks downstream readers when generating things with latest FreeSurfer.

For example, the output of this command:

$ mri_watershed -useSRAS -surf ./fsaverage \
    ~/applications/freesurfer-6/subjects/fsaverage/mri/T1.mgz ws.mgz

On Freesurfer 6 produces a file fsaverage_brain that starts with:

ÿÿþcreated by larsoner on Tue Aug 20 13:55:57 2019

^@^@(^B^@^@P^@À^K^[SÁ<89>8:Bªñ»A<98>ü<98>...

And on latest Freesurfer dev starts with:

ÿÿþcreated by larsoner on Tue Aug 20 13:59:19 2019
^@^@(^B^@^@P^@À^DìðÁ<8c>ËúB¨<93><A<9a>;˾¾^TB^ZÓnB<85>äÄÁ<9a>^VRB^X·WA<9c>...

There is a newline missing, which is problematic because downstream readers such as nibabel assume the procedure for reading should be the following:

  1. Read magic
  2. Read stamp
  3. dump the next line
  4. Read dimensionality
  5. Read data

Because of the missing newline, on latest freesurfer the fobj.readline() dump-the-next-line step must be removed in order for the file to be read properly. This behavior should probably be reverted (?) so that downstream readers don't suffer from this problem.

I assume this change is buried somewhere in the history of MRISwriteTriangularSurface, but don't actually know:

https://github.com/freesurfer/freesurfer/blame/5e5b72f4871bfffe0246aed964a588b97ace65fc/utils/mrisurf_io.cpp#L5837-L5840

My best guess is that in the old code (about a year ago when it was in mrisurf.c?), the current_date_time() maybe had a newline on it?

  time_str = current_date_time();
  ...
  fwrite3(TRIANGLE_FILE_MAGIC_NUMBER, fp);
  fprintf(fp, "created by %s on %s\n", user, time_str);

This would have caused two newlines to be written before the data. But this is just speculation on my part!

@ahoopes
Copy link
Member

ahoopes commented Jan 3, 2020

You're right - the extra newline was accidentally removed as a result of the c++ refactor, and this change only got caught by external surface readers. The newline has been re-added as of 2aae19d, and any previously "broken" surface files can be correctly resaved with an updated mris_convert (or one from a previous stable release).

@ahoopes ahoopes closed this as completed Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants