BUG: Fix incorrect reading of MINC2 files, convert RAS coordinates to LPS

[gdevenyi]

Replaces #147

PR Checklist

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)
• Added test (or behavior not changed)
• Updated API documentation (or API not changed)
• Added license to new files (if any)
• Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

[gdevenyi]

TODO:

• port TransformMINC fixes
• add tests
  • read in MINC file, read in NIFTI file, confirm identical with internal representation
• wrap in some kind of ITK legacy warning

[seanm]

I'd be interested to try this in our app when you think it's in shape...

[vfonov]

I'll try to make changes....

[vfonov]

TODO:

• port TransformMINC fixes

• add tests

  • read in MINC file, read in NIFTI file, confirm identical with internal representation

• wrap in some kind of ITK legacy warning

https://github.com/vfonov/ITK/commits/MINC_IMAGE_RAS_LPS_VF/ - here is my work in progress, i am going to add tests for all the new cases...

[blowekamp]

Would any of the enums enums for the AnatomicalOrientaion PR be useful here?

I am seeing a lot of LPS and RAS terms in this PR which can be little ambiguous. At lease the public interface of "RAS_to_LPS" the setter getter could be clarified and be make unambiguous.

[vfonov]

At lease the public interface of "RAS_to_LPS" the setter getter could be clarified and be make unambiguous.

What would make it less ambiguous?

[vfonov]

OK, How do i push my changes here from https://github.com/vfonov/ITK/tree/MINC_IMAGE_RAS_LPS ?

[jhlegarreta]

Pushing to Gabriel's branch should work:

git push gdevenyi MINC_IMAGE_RAS_LPS

The above assumes that the you have created your MINC_IMAGE_RAS_LPS branch from gdevenyi's branch. If not, I believe

git remote add gdevenyi https://github.com/gdevenyi/ITK.git
git fetch gdevenyi
git checkout MINC_IMAGE_RAS_LPS
git branch -u gdevenyi/MINC_IMAGE_RAS_LPS
git push gdevenyi MINC_IMAGE_RAS_LPS

should have the desired effect. If you run into trouble, you can start a fresh branch (assuming you rename your local MINC_IMAGE_RAS_LPS to something else to avoid issues) and cherry-pick your commit:

git remote add gdevenyi https://github.com/gdevenyi/ITK.git
git fetch gdevenyi
git checkout -b gdevenyi MINC_IMAGE_RAS_LPS
git cherry-pick 7688f522b685eb4ace0b9753f0bbf145fd6389bb
git push gdevenyi MINC_IMAGE_RAS_LPS

[vfonov]

git push gdevenyi MINC_IMAGE_RAS_LPS

ERROR: Permission to gdevenyi/ITK.git denied to vfonov.

[jhlegarreta]

ERROR: Permission to gdevenyi/ITK.git denied to vfonov.

@gdevenyi should be able to grant permissions to push to his ITK fork on GitHub, which should fix the issue.

[gdevenyi]

I have invited @vfonov

[vfonov]

So itkSPSAOptimizerTest fails on Windows, do we need to do anything about that?

[blowekamp]

This newly merged class should provide some documentation to better describe the coordinate changes in an unambiguous fashion: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/include/itkAnatomicalOrientation.h

[vfonov]

so, where does this new class apply in the proposed change?

[blowekamp]

so, where does this new class apply in the proposed change?

Please document what you mean by RAS in the unambiguous terminology described in that class.

[vfonov]

so, where does this new class apply in the proposed change?

Please document what you mean by RAS in the unambiguous terminology described in that class.

So, it's a documentation question - i.e I should updated documentation of the MINC reader Module?

[vfonov]

@gdevenyi - can you make changes requested by @blowekamp ? I do not understand what's required here.

[jhlegarreta]

Thanks for doing this @gdevenyi. An in-line style comment.

[jhlegarreta]

Left some style-related comments. There is a conflict that needs to be resolved since the code base has undergone some extensive style-related changes in the meantime.

While at it, stick to the commit message conventions: use sentence case (capitalize first letter), finish commit message BODY explanation with a period.

[blowekamp]

@vfonov @gdevenyi It is my understanding that this PR has conflicts due to all the recent activity. Thank you for the contribution and maintaining MINC for ITK.

Can I help updating the PR, and trying to address some of these style issues?

Also, I think we should do this change in 2 steps. This first is adding the LPS as an option preserving compatibility. And is a second PR, change the default? Maybe introduce an environment variable for this setting too?

Let me know if I can help make changes to get this PR through.

[gdevenyi]

Can I help updating the PR, and trying to address some of these style issues?

Yes please. I would very much appreciate the help.

This contribution to ITK has been 6 years in the making with the original technical solution remaining unchanged. I'm a generalist and ITK is a C++-expert level codebase, I'm a monkey at a typewriter here trying to keep up with the high churn and level of abstraction that this codebase demands. The complexity along with the serial reviews have made it very difficult.

I would very much appreciate any help you can offer.

[vfonov]

So, I don't understand - what needs to be done right now. Do we neeed:

1. rebase to the current ITK master?
2. address various style comments by @jhlegarreta regarding "ITK SWG"
3. fix the "This branch has conflicts that must be resolved" message?
4. fix just the "bool mRAStoLPS" variable (that's what shows up in "requested" changes .

[blowekamp]

So, I don't understand - what needs to be done right now. Do we neeed:

1. rebase to the current ITK master?
2. address various style comments by @jhlegarreta regarding "ITK SWG"
3. fix the "This branch has conflicts that must be resolved" message?
4. fix just the "bool mRAStoLPS" variable (that's what shows up in "requested" changes .

Yes, I thin that is a summary of what needs to be done. I am also going to separate the change in behavior for another PR, to make review easier. I am going to start working on update the PR branch.

[seanm]

Glad to see this work. Again, when it's in better shape, I'll give it a try in our app, which has various tests of its own that might exercise some of this.

[blowekamp]

I have updated the PR. For now I removed the CMake option, and have kept the behavior the same as it was. This can be changed in another PR.

Is it useful to still write with the the corrected coordinates? I am thinking that the correct cordinates should always be written no, and the flag is used then loading.

The "RAStoLPS" flag was not immediately clear to be what it did. Perhaps something line "ApplyCoordiateConversionForITK" would be more descriptive?

Also is there meta-data written in the transform to indicate that coordinates have been corrected?

[gdevenyi]

Is it useful to still write with the the corrected coordinates? I am thinking that the correct cordinates should always be written no, and the flag is used then loading.

The correct coordinates for a MINC file are RAS on-disk. This dynamic conversion must happen during both read and write into ITK.

"ApplyCoordiateConversionForITK" would be more descriptive?

Yes, this rename is reasonable.

Also is there meta-data written in the transform to indicate that coordinates have been corrected?

Similar to the image format, there is only one right way for the coordinate to be represented in MINC-land so there's no need to note correction

Ref: https://en.wikibooks.org/wiki/MINC/SoftwareDevelopment/MINC2.0_File_Format_Reference

[seanm]

We will test this at the hackathon tomorrow.

If you have time to rebase it against master that'd be great.

[jhlegarreta]

If you have time to rebase it against master that'd be great.

Done.

[seanm]

I can say that building our own app against this PR works, all our tests still pass too. That's a low bar mind you, we don't ourselves have extensive MINC testing (we mostly rely on upstream).

Are there sample files should should behave differently before and after this change?

[gdevenyi]

I can say that building our own app against this PR works, all our tests still pass too. That's a low bar mind you, we don't ourselves have extensive MINC testing (we mostly rely on upstream).

Are there sample files should should behave differently before and after this change?

The PR includes the changes to the tests which show how things change.

Fundamentally, this fix allows MINC and non-minc files to be mixed together inside a ITK pipeline since they will now correctly have the same internal coordinate system representation. Before this PR, the MINC files would be flipped around such that they were self-consistently wrong in terms of input/output, but couldn't cross the boundary to another file format correctly.

[dzenanz]

Gabriel, Vladimir, and Brad, this PR seems close. What is left to do besides removing WIP from one of those commits? I would like this PR to be merged sooner rather than later.

[gdevenyi]

What is left to do besides removing WIP from one of those commits? I would like this PR to be merged sooner rather than later.

@dzenanz if the developers who were asking for style changes are happy with the commits now I believe this is ready.

[jhlegarreta]

I have removed the WIP prefix from the commit and provided some best guess context in the commit message as to the change of the commit. Please feel free to modify if required.

Edit: I now see that the commit (6e75f1f) at issue removes lines of code that were introduced in another commit (50f6526) in this PR. Those changes should be merged and the unnecessary commit be removed, but given the state of things, I would prefer to avoid further difficulties with this PR. So this is ready to go for me.

[dzenanz]

Thank you all!

[gdevenyi]

Thank you all 🎉🎉🎉