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

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

Closed

Conversation

gdevenyi
Copy link
Contributor

@gdevenyi gdevenyi commented Nov 7, 2018

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
Copy link
Contributor Author

gdevenyi commented Nov 7, 2018

@vfonov
Copy link
Contributor

vfonov commented Nov 7, 2018

What about minc transform?

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Nov 7, 2018

@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
Copy link
Contributor Author

gdevenyi commented Nov 7, 2018

@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
Copy link
Contributor Author

gdevenyi commented Nov 9, 2018

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 gdevenyi changed the title BUG: Fix MINC read/write to convert from/to RAS coordinate system WIP: Fix MINC read/write to convert from/to RAS coordinate system Nov 9, 2018
@gdevenyi
Copy link
Contributor Author

gdevenyi commented Nov 9, 2018

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.

@vfonov
Copy link
Contributor

vfonov commented Nov 9, 2018

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.

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Nov 9, 2018

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

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Nov 12, 2018

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

@vfonov
Copy link
Contributor

vfonov commented Nov 13, 2018

Sounds like a plan

@gdevenyi
Copy link
Contributor Author

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

@vfonov
Copy link
Contributor

vfonov commented Nov 14, 2018

nlfit <-> antsApplyTransforms probably will not work.

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Nov 14, 2018

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
Copy link
Contributor Author

gdevenyi commented Nov 16, 2018

PR #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
Copy link
Contributor Author

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

@thewtex thewtex requested a review from vfonov November 26, 2018 20:27
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I am not too familiar with MINC format, I only have minor remarks.

Modules/IO/MINC/src/itkMINCImageIO.cxx Outdated Show resolved Hide resolved

Matrix< double, 3,3 > RAS_affine_transform;
Matrix< double, 3,3 > LPS_affine_transform;
RAS_affine_transform.Fill(0.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not needed.

RAS_affine_transform.SetIdentity();
//MINC stores transforms in RAS, need to convert to LPS for ITK
Matrix< double, 3,3 > RAS_tofrom_LPS;
RAS_tofrom_LPS.Fill(0.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed, I think.

Matrix< double, 3,3 > RAS_tofrom_LPS;
Matrix< double, 3,3 > RAS_affine_transform;
Matrix< double, 3,3 > LPS_affine_transform;
RAS_tofrom_LPS.Fill(0.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably redundant.

@gdevenyi
Copy link
Contributor Author

Thanks @dzenanz I have removed the extraneous Fills.

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Dec 3, 2018

@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?

@thewtex
Copy link
Member

thewtex commented Dec 3, 2018

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

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Dec 3, 2018

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

@hjmjohnson
Copy link
Member

@gdevenyi Is this PR still relavant?

@stale stale bot removed the status:Backlog Postponed without a fixed deadline label Aug 2, 2022
@gdevenyi
Copy link
Contributor Author

gdevenyi commented Aug 2, 2022

@gdevenyi Is this PR still relavant?

Yes, the MINC reader is still incorrect, and the coordinate system conversion fixes it.

@hjmjohnson
Copy link
Member

@gdevenyi Could you provide a roadmap for what you think needs to be done still? There may be students looking for projects this fall, and I'll see if I can convince one of them to look at this.

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Aug 2, 2022

@hjmjohnson

Current state as I recall, fixed are

  • images load and save properly now
  • affine transforms read and write properly (both MINC/ITK push/pull definitions handled and RAS/LPS coordinates converted)
  • warp transforms read and write properly (both MINC/ITK push/pull definitions handled and RAS/LPS coordinates converted)

as I recall the blockers were

  1. How to handle the breaking change for downstream dependent tools, right now there's a bool option in the loader, but I don't see how to access given how most tools use a generic reader, I think it should be a compile-time switch instead, I'm not skilled enough in CMAKE to make that happen
  2. How to handle other voxel formats which have directional information, in particular DTI

@vfonov
Copy link
Contributor

vfonov commented Aug 2, 2022

There is currently 4 different ways how Analyze file format can be handled, just make the same thing for MINC/XFM reader writer : https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/NIFTI/include/itkNiftiImageIO.h#L42

@vfonov
Copy link
Contributor

vfonov commented Aug 11, 2022

It looks like the origin is incorrect when RAS to LPS is needed

@gdevenyi
Copy link
Contributor Author

Good catch @vfonov I'll take a look

@vfonov
Copy link
Contributor

vfonov commented Aug 12, 2022

I am trying to make a new pull on the latest codebase: https://github.com/vfonov/ITK/tree/MINC_RAS_LPS_v2 (WIP)

@gdevenyi
Copy link
Contributor Author

It looks like the origin is incorrect when RAS to LPS is needed

@vfonov The Origin Read/Write will need negative applied to the X and Y dimensions to go from R->L and A->P

@vfonov
Copy link
Contributor

vfonov commented Aug 18, 2022

It looks like the origin is incorrect when RAS to LPS is needed

@vfonov The Origin Read/Write will need negative applied to the X and Y dimensions to go from R->L and A->P

So, most things are fixed in https://github.com/vfonov/ITK/tree/MINC_RAS_LPS_v2 , we need to make more tests there.
Also, the TransformationMINC for RAS->LPS is not quite complete .

@gdevenyi
Copy link
Contributor Author

gdevenyi commented Jun 19, 2024

Can we perhaps revive this to integrate this breaking change for ITKv6?

@dzenanz @vfonov

@vfonov
Copy link
Contributor

vfonov commented Jun 19, 2024

I already forgot what needs to be done. Do we want to, essentially make MINCs behave as if they were NIFTIs ?

@seanm
Copy link
Contributor

seanm commented Jun 19, 2024

Can we perhaps revive this to integrate this breaking change for ITKv6?

@gdeveny Can you rebase on master and fix the conflicts? I'd be interested to see how/if this breaks our app (Brainsight)...

@gdevenyi
Copy link
Contributor Author

I already forgot what needs to be done. Do we want to, essentially make MINCs behave as if they were NIFTIs ?

The core issue is MINC's internal coordinates are RAS = +X+Y+Z, but the ITK internal Image coordinate system is LPS = +X+Y+Z. MINC-IO in ITK loads the files not converting the coordinate system, making it impossible to mix MINC and any other image loaded by ITK IO.

This is not "behave as if they were NIFTIs" but rather, "behave like every other image loaded by ITK ImageIO"

@gdeveny Can you rebase on master and fix the conflicts? I'd be interested to see how/if this breaks our app (Brainsight)...

Will do.

This will impact things if you were special casing anything about MINC, otherwise it should just make things load properly coordinate wise.

@dzenanz
Copy link
Member

dzenanz commented Jun 19, 2024

Why not just document (via direction matrix) that the image is in RAS coordinate system?

@seanm
Copy link
Contributor

seanm commented Jun 19, 2024

This will impact things if you were special casing anything about MINC, otherwise it should just make things load properly coordinate wise.

It's been forever since I touched our MINC reading code (via ITK), but maybe we are doing some flipping, because no one, internally or externally, has ever reported Brainsight loading any MINC files flipped.

@gdevenyi
Copy link
Contributor Author

Why not just document (via direction matrix) that the image is in RAS coordinate system?

Is this a suggestion to address this in code somehow? I'm not deeply familiar with ITK internals, so I'll need some hints.

My overall goal here was to bring MINC to first-class support in terms of it being treated equally and interchangeably with any other image loaded via ImageIO. Right now, I can't do this due to the misinterpreted coordinates.

@dzenanz
Copy link
Member

dzenanz commented Jun 19, 2024

I did not take a close look at the code. If you are shuffling array elements to "conform to LPS" standard, you don't need to do that. I see that you are fiddling with orientation matrices, so this PR might already be doing what I suggested.

@vfonov
Copy link
Contributor

vfonov commented Jun 19, 2024

As far as I remember now the issue is with transformations, in particular with non-linear _grid files. That would need to be recomputed via F∘T∘F transformation on loading and saving any such file.

@gdevenyi
Copy link
Contributor Author

As far as I remember now the issue is with transformations, in particular with non-linear _grid files. That would need to be recomputed via F∘T∘F transformation on loading and saving any such file.

Correct, this was the last outstanding thing that needs fixing in this PR in a performant way.

@gdevenyi
Copy link
Contributor Author

Rebasing was too difficult so I manually ported the changes into the new PR #4864

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