ITK icon indicating copy to clipboard operation
ITK copied to clipboard

WIP: Fix MINC read/write to convert from/to RAS coordinate system

Open gdevenyi opened this issue 5 years ago • 65 comments

Convert the coordinate system from RAS to LPS during MINC loading, and convert back on writing.

Also adds a setting to keep old configuration, so that MINC-based ITK tools (EZMinc) can maintain their original configuration and return sensible coordinate values in MINC-space.

Paging @vfonov

gdevenyi avatar Nov 07 '18 18:11 gdevenyi

Originally discussed at https://insightsoftwareconsortium.atlassian.net/browse/ITK-3503?attachmentSortBy=fileName

gdevenyi avatar Nov 07 '18 19:11 gdevenyi

What about minc transform?

vfonov avatar Nov 07 '18 19:11 vfonov

@vfonov I have the code for converting the MINC affine to from RAS to LPS, I was going to do a seperate PR. Would you rather I roll that into this?

gdevenyi avatar Nov 07 '18 20:11 gdevenyi

@vfonov MINC vs ITK also defines its transforms in opposite directions, do you want that added to the reader, or leave that to the user?

gdevenyi avatar Nov 07 '18 20:11 gdevenyi

I have added the conversion for Affine transforms.

I'm not quite sure where to add the switch to disable this conversion to support backwards compatibility.

Can anyone give some guidance here?

gdevenyi avatar Nov 09 '18 22:11 gdevenyi

My transform work isn't right. It used to be combined with inverting the affines so that MINC and ITK transforms went in the same direction. I backed that part out and this needs some fixes as a result.

gdevenyi avatar Nov 09 '18 22:11 gdevenyi

one of the main reason for me to have minc interface for ITK is to be able to run ANTs & Elastix. I think we should run extensive checks to make sure that this conversion does not screw up their results.

vfonov avatar Nov 09 '18 22:11 vfonov

Totally agree. On the walk home I wrote up a bunch of tests in head to ensure this works properly. Will enumerate later.

gdevenyi avatar Nov 09 '18 23:11 gdevenyi

Given two files, in both MINC and NIFTI format: GAD

  • scan of me T1
  • convert to MINC with dcm2mnc, minc-toolkit/1.9.16
  • convert to NIFTI with dcm2niix/1.0.20171215

mni_icbm152_t1_tal_nlin_asym_09c

  • original MINC version
  • convert to NIFTI with mnc2nii

Tests to do:

  • minc-to-minc affine registration with antsRegistration
  • nifti-to-minc affine registration with antsRegistration
  • minc-to-nifti affine registration with antsRegistration
  • bestlinreg minc-to-minc
  • save transforms as both ants and minc format
  • apply all transforms using antsApplyTransforms
  • apply minc-style transforms with mincresample
  • ConvertImage to/from minc/nifti

gdevenyi avatar Nov 12 '18 22:11 gdevenyi

Sounds like a plan

vfonov avatar Nov 13 '18 02:11 vfonov

Additional tests:

  • antsRegistration SyN to produce nonlinear
  • save as minc/nifti
  • confirm forward/reverse transformations also work as expected with antsApplyTransforms and mincresample
  • nlfit to produce nonlinear
  • confirm forward/reverse work with antsApplyTransforms

gdevenyi avatar Nov 14 '18 20:11 gdevenyi

nlfit <-> antsApplyTransforms probably will not work.

vfonov avatar Nov 14 '18 20:11 vfonov

All conversions and affine registrations now pass my listed tests.

The following still don't work

  • nlfip -> antsApplyTransform
  • mincresample applying the NLIN fields produced by ANTs

ITK and MINC have opposite transform directions, and nlpfit doesn't produce an inverse warp field and doesn't handle the invert flag of an XFM grid transform properly (yet...)

I think we have the issue that in grid-based transforms, the X and Y vector components point in opposite directions when produced in minc-land or ITK land.

Questions

  1. Should we flip all the voxel vectors?
  2. Can anyone suggest an efficient way to do this?
  3. @vfonov the TransformMINC doesn't have an option to not flip the transforms, do you need it?

gdevenyi avatar Nov 14 '18 21:11 gdevenyi

PR https://github.com/InsightSoftwareConsortium/ITK/pull/194 addresses the issue with MINC NLIN transforms only being generated one-way and ITK not properly handling the invert flag on such transforms.

Still remaining is I need to figure out an efficient method to voxel-wise multiply the incoming grid file by (-1,-1,1) to flip the RAS vector components to LPS vectors.

gdevenyi avatar Nov 16 '18 20:11 gdevenyi

Added patch now properly handles Grid transforms to/from MINC land.

My local tests of

  • mincresample applying ITK generated transforms
  • antsApplyTransforms applying grid transforms generated by nlpfit

Now work properly.

The question that remains is should ITK also invert all transforms it produces (and handle inversion on reading) so that the directionality of MINC vs ITK transforms is also transparently handled.

This patch also works properly on top of/with #194

gdevenyi avatar Nov 26 '18 20:11 gdevenyi

Thanks @dzenanz I have removed the extraneous Fills.

gdevenyi avatar Nov 30 '18 16:11 gdevenyi

@thewtex I think this change is breaking the current tests, is that correct? If so, any hints as to how to I would go about sorting that out?

gdevenyi avatar Dec 03 '18 16:12 gdevenyi

@gdevenyi there are 51 minc tests failing on all platforms. Do the tests all pass locally?

thewtex avatar Dec 03 '18 20:12 thewtex

@thewtex that's what I'm asking? how do I go about checking that locally :)

gdevenyi avatar Dec 03 '18 20:12 gdevenyi

@gdevenyi ah, turn on BUILD_TESTING in your ITK build's CMake configuration, re-build, then run ctest from the build tree.

thewtex avatar Dec 03 '18 20:12 thewtex

All the test are failing with errors like this, with can't read a file.

/scratch/ITK-build/bin/ITKIOMINCTestDriver "--compare" "/scratch/ITK-build/ExternalData/Modules/IO/MINC/test/Input/t1_z+_ushort_trans.mnc" "/scratch/ITK-build/Testing/Temporary/t1_z+_ushort_trans.mnc" "itkMINCImageIOTest4" "/scratch/ITK-build/ExternalData/Modules/IO/MINC/test/Input/t1_z+_ushort_trans.mnc" "/scratch/ITK-build/Testing/Temporary/t1_z+_ushort_trans.mnc" "427590.7957" "-8.195997123" "72.45943721" "-3.148635961"
Image:/scratch/ITK-build/ExternalData/Modules/IO/MINC/test/Input/t1_z+_ushort_trans.mnc sum=427590.7957 COM=[8.195997127, -72.45943721, -3.148635962]
Total mx mismatch:8.196
Image:/scratch/ITK-build/ExternalData/Modules/IO/MINC/test/Input/t1_z+_ushort_trans.mnc sum=427590.7957 COM=[8.195997127, -72.45943721, -3.148635962]
Total mx mismatch:8.196
/home/cic/devgab/projects/src/ITK.github/Modules/ThirdParty/MINC/src/libminc/libsrc2/volume.c:1400 (from MINC): Unable to open file '/scratch/ITK-build/Testing/Temporary/t1_z+_ushort_trans.mnc'
Exception detected while reading /scratch/ITK-build/ExternalData/Modules/IO/MINC/test/Input/t1_z+_ushort_trans.mnc : itk::ERROR: MINCImageIO(0x23e97a0): Could not open file "/scratch/ITK-build/Testing/Temporary/t1_z+_ushort_trans.mnc".<DartMeasurement name="BaselineImageName" type="text/string">INVALID_BASELINE_GIVEN</DartMeasurement>

It looks to me like the tests are getting setup slightly wrong maybe and the input files can't be found? Or am I misreading this?

gdevenyi avatar Dec 04 '18 17:12 gdevenyi

The test runs, then it tries to compare a test output file, /scratch/ITK-build/Testing/Temporary/t1_z+_ushort_trans.mnc to a baseline file, but the test output file not exist. What is the test output file not being written?

thewtex avatar Dec 04 '18 17:12 thewtex

Oops. I see it now. Its testing COM in RAS space, and that's wrong now :+1:

Patch incoming

gdevenyi avatar Dec 04 '18 18:12 gdevenyi

All passing!

@vfonov the only remaining question is: do you need a flag to disable the correction of coordinate spaces for the MINCTransform code? I'm not sure how I would do that right now...

gdevenyi avatar Dec 04 '18 20:12 gdevenyi

yes, i would say that it is critical, and it have to be very explicit to Enable new behaviour

vfonov avatar Dec 04 '18 20:12 vfonov

All passing!

@vfonov the only remaining question is: do you need a flag to disable the correction of coordinate spaces for the MINCTransform code? I'm not sure how I would do that right now...

Also, i would like to completely disable the new behaviour where COM location is in different coordinates then what mincstats would report.

vfonov avatar Dec 04 '18 20:12 vfonov

@vfonov the disable coordinate conversion is already added at https://github.com/InsightSoftwareConsortium/ITK/pull/147/files#diff-32784fd54fb5bc9cf57e46a4395d785bR94

I have to strongly disagree that these fixes should be opt-in behaviour. Right now the ITK MINC reader is wrong. The coordinates are incorrect according to the internal ITK standard when loading files, making it impossible to mix MINC with any other filetype. These changes make MINC transparently work with the other filetypes ITKIO supports.

To provide backwards compatibility I will figure out how to add a flag to TransformMINC to disable this as well.

gdevenyi avatar Dec 04 '18 21:12 gdevenyi

If it was easy to disable, Why did you have to change the tests that were failing? I want to keep using old behavior by default.

vfonov avatar Dec 04 '18 21:12 vfonov

@vfonov The tests were updated as the COM that were hard coded in those tests use the RAS coordinates. Now that the coordinates of the loaded MINC files are properly converted to ITK LPS space, the x and y values need to be -ve'd to have them correspond to the values printed by ITK's internal calculations.

@thewtex I need to add a feature/option to disable the RAS->LPS conversion for the TransformMINC work. Do you have a suggestion as to how I could add something that would be accessible? It looks like a common way to write out transforms is a TransformFileWriterTemplate which determines the underlying code based on the filename? If I add a option at the TransformMINC level can that propagate upwards so that its accessible?

gdevenyi avatar Dec 05 '18 15:12 gdevenyi

@gdevenyi Yes, if an option is available on TransformMINC, that can be set, then passed to TransformFileWriterTemplate with SetTransformIO.

thewtex avatar Dec 05 '18 15:12 thewtex