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

FIX:fieldmaps #565

Merged
merged 5 commits into from Jun 18, 2017
Merged

FIX:fieldmaps #565

merged 5 commits into from Jun 18, 2017

Conversation

jdkent
Copy link
Collaborator

@jdkent jdkent commented Jun 16, 2017

Fixes #563

@effigies
Copy link
Member

That error suggests that the fieldmaps coming in are 16-bit integers. You're now applying a floating point multiplication, which should result in a floating point fmapdata. Creating the image with the original header will result in casting to int16 again on writing the file...

Can you make sure that you're getting output files that are interpreted as floats? Maybe run nib-ls -s on your inputs and outputs.

@jdkent
Copy link
Collaborator Author

jdkent commented Jun 16, 2017

Of course. Input to 'torads' from 'fmapmrg'

nib-ls -s sub-GE161_ses-activepre_fieldmap_ras.nii.gz 
sub-GE161_ses-activepre_fieldmap_ras.nii.gz int16 [ 64,  64,  37] 3.44x3.44x4.00    [150001] [-2.2e+02, 2.2e+02]

Input to 'tohz' from 'torads'

nib-ls -s sub-GE161_ses-activepre_fieldmap_ras_rad.nii.gz 
sub-GE161_ses-activepre_fieldmap_ras_rad.nii.gz int16 [ 64,  64,  37] 3.44x3.44x4.00    [151552] [-3.1, 3.1]

output from 'tohz'

nib-ls -s sub-GE161_ses-activepre_fieldmap_ras_rad_unwrapped_hz.nii.gz 
sub-GE161_ses-activepre_fieldmap_ras_rad_unwrapped_hz.nii.gz float32 [ 64,  64,  37] 3.44x3.44x4.00    [31739] [-3.2e+02, 5.1e+02]

I'm not getting floats out of most of these, so I should be changing the data-type of my fieldmaps before I submit them to fmriprep?

@jdkent
Copy link
Collaborator Author

jdkent commented Jun 17, 2017

when I change my fieldmap datatype to float before I pass it in to fmriprep the error doesn't occur. The int datatype may be due to how our fieldmaps are reconstructed from dicoms. The fieldmap and magnitude image are put into the same nifti file resulting in a 2 volume nifti. We split them (using fslroi) and then do processing from there. So instead of the commit I proposed, would it make more sense to do a datatype check, and if that fails print a warning and convert the image to float? Or is this too much of a niche case? I am unsure if my fieldmap being int type suggests a deeper problem with how I'm reconstructing my data and should be fixed beforehand.

@effigies
Copy link
Member

Sorry, I didn't mean to suggest that users need to coerce their inputs to amenable data types. I was wondering about what the consequences of getting int16 inputs would be on the outputs, given that we're passing the original header to the new image, which means we'll be saving floats as int16. Basically, I was considering whether we should add a step to ensure that the output is saved as float32, which would look like:

out_img = nb.Nifti1Image(fmapdata, fmapnii.affine, fmapnii.header)
out_img.set_data_dtype('float32')
out_img.to_filename(out_file)

It appears that you are getting output images that have int16 data types on disk, but interpreted as floats (note that the bounds on your file in radians ≈±π).

So basically it's a question of whether we want to use the precision of the inputs. I lean toward setting the output type to float for consistency, but I'm open to arguments.

@effigies
Copy link
Member

As to whether your (pre-)preprocessing needs adjustment, that's hard for me to say. My working knowledge of fieldmaps does not extend to what units and datatypes to expect out of your DICOMs; perhaps @oesteban or @chrisfilo can speak more authoritatively, here.

@jdkent
Copy link
Collaborator Author

jdkent commented Jun 17, 2017

I agree with your assessment for consistency, I made the changes per your suggestion, but I just realized I need to wait for the fsl version to update for this to pass the tests :/

@effigies
Copy link
Member

Looks reasonable. @oesteban?

@oesteban
Copy link
Member

Yes, this is very reasonable. The preprocessing should be robust to the integer range of the image. Good catch!

@oesteban
Copy link
Member

@jdkent what you mean by

I need to wait for the fsl version to update for this to pass the tests

@jdkent
Copy link
Collaborator Author

jdkent commented Jun 17, 2017

@oesteban sorry, I should have linked it. see #566. I don't think neurodebian kept fsl-core 5.0.9-1 to download, so I had to update it to fsl-core 5.0.9-4 for the docker container to be successfully created

@oesteban
Copy link
Member

Thanks!

That's merged now :)

@effigies
Copy link
Member

Alright. Looks good. Thanks a lot!

@effigies
Copy link
Member

Wait a second. I think your original fix disappeared. We're back to fmapdata *=, which I think should still be susceptible to the TypeError when you pass in your original files (I think). Can you verify that this works on your original data that was causing the error?

@jdkent
Copy link
Collaborator Author

jdkent commented Jun 17, 2017

Sorry! Nope it was still causing the error, added the original fix back in.

@effigies effigies merged commit 1bba08e into nipreps:master Jun 18, 2017
@jdkent jdkent deleted the fix/fieldmaps branch June 18, 2017 00:57
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.

None yet

3 participants