-
Notifications
You must be signed in to change notification settings - Fork 262
NF: add support for V4 of PARREC format #277
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
Conversation
Looks reasonable to me. I assume you derived the fake example from a V4 format file you have on hand? BTW @mrjeffs plans to hopefully obtain some sample images that we could share as test cases for nibabel. If we can get them, it probably won't be for at least a couple of weeks, so it's not worth delaying release for. |
Yes, the fake example is from looking at some of the Rosetta bit project data. I think it's representative, but I didn't want to include the Rosetta bit data because of the license. |
Any way we could contact them to ask if we can re-license one of the files under MIT/BSD? I'm not familiar with the project, how big or flexible it is, etc. |
Discussion here - https://www.nitrc.org/forum/forum.php?thread_id=4975&forum_id=4571 One option is to use a sample of their data in a separate test images repository, so their license would only apply to the test data, not to the nibabel source repo. |
Didn't you set up some form of test data repo? That would presumably allow us to specify the license of subsets of the data. |
Yeah - that was the idea - experimenting with https://bitbucket.org/nipy/rosetta-samples as submodule. |
Branch here: https://github.com/matthew-brett/nibabel/tree/rosetta-tests We differ a lot on the affines, I think we'll need your viewer to check who is right. |
Hmm... I guess this is a blocker for release, then, since it tests the |
It looks like the affines are OK but the converted images differ so it is difficult to test in an automated way. I suggest we release with this PR, and work on validating on these images later. |
To check the affines I merged:
and then used this script: https://gist.github.com/matthew-brett/667dd9e7516aaa7109cf to view the Rosetta nifti files and our converted files in real space. |
I don't have a V4 .PAR file on hand to test this, but I think the last 3 entries of
edit: actually the first two of those were added in V4.1, and the last in V4.2 |
V4.1 added the additional diffusion gradient info over V4, while V4.2 added the ASL-related label type field. 1.) in 2.) in The changes to support all the way back to V3 are a bit more substantial and I don't have any files that old to test with. |
Actually, we don't need to modify |
Add PARREC 4.1 support from information by Gregory Lee at nipy#277
Gregory - would you mind checking my fake V4.1 PAR file is what you expected? Does this look right to you? |
V4 appears to differ only in having fewer general and image information fields.
Add PARREC 4.1 support from information by Gregory Lee at nipy#277
c79c134
to
eb9f499
Compare
looks good to me |
MRG: add support for V4, 4.1 of PARREC format Versions 4 and 4.1 seem to differ only in in lacking a few fields of the 4.2 PAR header.
Add PARREC 4.1 support from information by Gregory Lee at nipy#277
MRG: add support for V4, 4.1 of PARREC format Versions 4 and 4.1 seem to differ only in in lacking a few fields of the 4.2 PAR header.
V4 appears to differ only in having fewer general and image information fields.