Skip to content

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

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

Merged
merged 4 commits into from
Jun 27, 2016

Conversation

jaeilepp
Copy link
Contributor

Follow up to #460.
I didn't notice that the data format is actually different when reading new quad files. I could not find a file in freesurfer that would use this format so I couldn't add a test for it.

@coveralls
Copy link

coveralls commented Jun 21, 2016

Coverage Status

Coverage decreased (-0.004%) to 96.107% when pulling d24cb38 on jaeilepp:fix-surf into f4a3e0a on nipy:master.

@effigies
Copy link
Member

This should be an old-style quad file (mentioned here). Not sure if anybody's seen a new-style quad. @Eric89GXL?

@larsoner
Copy link
Contributor

No idea, it took me quite a while to find the old one :) The best bet is probably to ping the FreeSurfer listserv.

@jaeilepp
Copy link
Contributor Author

This should be an old-style quad file (mentioned here). Not sure if anybody's seen a new-style quad.

I think that is a new quad file. But it indeed seems like quad files are not tested at all. Someone needs to update the testing dataset to make it possible to test them.

@agramfort
Copy link
Contributor

just make available a file and @matthew-brett will add it to the freesurfer data package

@jaeilepp
Copy link
Contributor Author

@matthew-brett
Copy link
Member

@jaeilepp - what's the license for that file?

@jaeilepp
Copy link
Contributor Author

It's part of the FS tutorial dataset, I suppose it's using the freesurfer license.

@matthew-brett
Copy link
Member

matthew-brett commented Jun 23, 2016 via email

@jaeilepp
Copy link
Contributor Author

It lives in the 'lightweight' tutorial dataset http://surfer.nmr.mgh.harvard.edu/fswiki/FsTutorial/Data

@matthew-brett
Copy link
Member

From tutorial_data/buckner_public_distribution.README.txt - appears to be derived from https://web.archive.org/web/20070711224104/http://www.fmridc.org/f/fmridc/78.html

But - the copy I see in tutorial_data/tutorial_subjs/bert/surf/lh.inflated.nofix from ftp://surfer.nmr.mgh.harvard.edu/pub/data/tutorial_data.tar.gz has a different hash from the one at https://github.com/mne-tools/mne-testing-data/tree/master/subjects/bert/surf - any ideas why? Are they meant to be the same file?

@matthew-brett
Copy link
Member

matthew-brett commented Jun 23, 2016

If the source is the FMRIDC, the sort-of license is:

For data housed at the Center, anyone has the right to publish findings 
based on these datasets. Papers whose results are based on datasets 
obtained from the Center should credit the authors of the original study
and acknowledge the Center and accession number of the dataset.

https://web.archive.org/web/20070630230750/http://www.fmridc.org/f/fmridc/help/faq.html#DataSharing

@jaeilepp
Copy link
Contributor Author

Oh, I just assumed it's the same file with the same name and all. Any idea @Eric89GXL ?

@larsoner
Copy link
Contributor

From the note it comes from FreeSurfer 3.0. We should probably ping the FreeSurfer folks to see if it's okay to relicense it to whatever form is necessary for nibabel.

@matthew-brett
Copy link
Member

Is the file version in the tutorial dataset OK for the test? In that case we could use that, because the license is reasonably clear.

@matthew-brett
Copy link
Member

The file at https://github.com/mne-tools/mne-testing-data/tree/master/subjects/bert/surf does have the same hash as freesurfer/subjects/bert/surf/lh.inflated.nofix from ftp://surfer.nmr.mgh.harvard.edu/pub/dist/freesurfer/3.0.3/freesurfer-Linux-rh7.3-stable-pub-v3.0.3-full.tar.gz

@jaeilepp
Copy link
Contributor Author

Is the file version in the tutorial dataset OK for the test?

Actually no. Seems that it is the same triangle type as the other files.
The file from ftp://surfer.nmr.mgh.harvard.edu/pub/dist/freesurfer/3.0.3/freesurfer-Linux-rh7.3-stable-pub-v3.0.3-full.tar.gz would work.

@matthew-brett
Copy link
Member

Sadly the freesurfer 3 archive is silent on the license for the file, as far as I can see.

@matthew-brett
Copy link
Member

How about the file subjects/bert/surf/lh.inflated.nofix in the current 5.3.0 freesurfer archive? I'm asking, because I'm preparing a question to the freesurfer mailing list, and I guess it's easier for them if the question refers to their latest release.

@larsoner
Copy link
Contributor

At least the MD5's don't match

33dd533812f168cddc464c7d64d69166  /home/larsoner/custombuilds/freesurfer/current/subjects/bert/surf/lh.inflated.nofix
82389cce73cc31ea59ab81bc24ff3160  /home/larsoner/custombuilds/mne-testing-data/subjects/bert/surf/lh.inflated.nofix

And the file sizes are 4863500 and 3543685, respectively.

@matthew-brett
Copy link
Member

Right - I knew they were different files, but I was wondering if the newest version from freesurfer 5.3.0 can still be used for the tests.

@larsoner
Copy link
Contributor

I was pretty sure I had to go back that far to find a quad file for testing MNE-Python. I just checked and can confirm that nibabel master cannot read the old file but can read the new file.

@matthew-brett
Copy link
Member

Great - thanks - I emailed the freesurfer list.

@matthew-brett
Copy link
Member

@matthew-brett
Copy link
Member

Latest nibabel master now has the updated submodule with the new data file at nibabel-data/nitest-freesurfer/subjects/bert/surf/lh.inflated.nofix.

@codecov-io
Copy link

codecov-io commented Jun 27, 2016

Current coverage is 94.21%

Merging #461 into master will increase coverage by 0.10%

@@             master       #461   diff @@
==========================================
  Files           160        160          
  Lines         21164      21173     +9   
  Methods           0          0          
  Messages          0          0          
  Branches       2265       2265          
==========================================
+ Hits          19919      19949    +30   
+ Misses          823        803    -20   
+ Partials        422        421     -1   

Powered by Codecov. Last updated by d6945c4...24a05ec

@coveralls
Copy link

coveralls commented Jun 27, 2016

Coverage Status

Coverage increased (+0.1%) to 96.207% when pulling 24a05ec on jaeilepp:fix-surf into d6945c4 on nipy:master.

@jaeilepp
Copy link
Contributor Author

I added the test, but now that I think about it, maybe I should have added a separate function for testing the quad file to avoid the needs_nibabel_data decorator. Wdyt?

@coveralls
Copy link

coveralls commented Jun 27, 2016

Coverage Status

Coverage increased (+0.1%) to 96.209% when pulling 9b66562 on jaeilepp:fix-surf into d6945c4 on nipy:master.

@jaeilepp
Copy link
Contributor Author

Okay, I made it a separate function. Ready for review.

@larsoner
Copy link
Contributor

LGTM

@agramfort
Copy link
Contributor

LGTM 2

@matthew-brett merge if you're happy

@matthew-brett
Copy link
Member

Great - thanks for slogging through.

@matthew-brett matthew-brett merged commit 2a9e96f into nipy:master Jun 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants