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.

[gdevenyi]

@blowekamp I have enhanced and rewritten the various comments and notes to better match the OrientFilter's language.

[blowekamp]

@blowekamp I have enhanced and rewritten the various comments and notes to better match the OrientFilter's language.

Great. Thank you for the clarity.

[gdevenyi]

Sorry, @vfonov whatever I did during the rebasing has lost you as author. Not sure how to fix that...

[jhlegarreta]

Commit messages do not comply with the ITK standards:
https://github.com/InsightSoftwareConsortium/ITK/pull/4864/checks?check_run_id=33807924553

Also, can this commit 9ecb71c be squashed and (its commit message removed) into 5a46692, please?

[gdevenyi]

Commit messages do not comply with the ITK standards: https://github.com/InsightSoftwareConsortium/ITK/pull/4864/checks?check_run_id=33807924553

I cannot satisfy both the request of @blowekamp for sufficent detail regarding this change and your request for single-line commit messages, as I will just violate the line length limit instead. What do you suggest?

Also, can this commit 9ecb71c be squashed and (its commit message removed) into 5a46692, please?

Done.

[jhlegarreta]

I cannot satisfy both the request of @blowekamp for sufficent detail regarding this change and your request for single-line commit messages, as I will just violate the line length limit instead. What do you suggest?

Yes, you can. Commit message subjects should be under 72 chars (although KWstyle seems to be more permissive in practice), and should briefly describe the change or proposed fix. The immediate next line should be empty, and the rationale for the change, bug and proposed fix should be in the commit message body:
https://docs.itk.org/en/stable/contributing/index.html#commit-messages
and
https://cbea.ms/git-commit/

Although many people disregard having a proper commit message body, it allows to explain the rationale of a change, the underlying issue, if any, and the proposed fix (including why other possibilities, if any, were discarded), and hence, it is essential to review a PR, and when inspecting history.

Thanks Gabriel.

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