ITK
ITK copied to clipboard
InsightSoftwareConsortium
Incorrect call when comparing matrices in nifti headers
Description
In this code transpose is used when inverse is intended:
https://github.com/InsightSoftwareConsortium/ITK/blob/7357008cbc2880cda302f00efd1a39b02aeffe08/Modules/IO/NIFTI/src/itkNiftiImageIO.cxx#L1978-L1982
It looks to me like the code was copy-pasted from above where transpose was intended but the code was not corrected:
https://github.com/InsightSoftwareConsortium/ITK/blob/7357008cbc2880cda302f00efd1a39b02aeffe08/Modules/IO/NIFTI/src/itkNiftiImageIO.cxx#L1943-L1946
Steps to Reproduce
I single stepped through while researching a different issue and realized this call was always false even when the matrices are identical. On inspection I could see these are full index to physical transforms and the transpose of one is not equal to the other.
Expected behavior
Code should implement intended behavior.
Actual behavior
Call always fails.
Reproducibility
Always.
Versions
Since Jul 4, 2020
The code in question comes from this commit:
https://github.com/InsightSoftwareConsortium/ITK/commit/34231b57021418fdb6afd4fcf5082a73b12969ed
Environment
All
Additional Information
N/A
@hjmjohnson Can you have a look at this? I would make a PR but I already have a fork of Slicer/ITK so it's not easy for me to make a PR to this repo. It should be just a one-word fix to change transpose to inverse on line 1980. thanks!
@pieper I'll have a look. The code should be changed or have a better comment at least.
I am pretty sure that qform_as matrix must be orthonormal. For an orthonormal matrix, the transpose is the inverse.
In any case, the performance benefits of transpose are probably not relevant in this code section.
Hans
Hi Hans - I see I had an incorrect link in the issue report before. I just edited it. You can see that line 1980 is very much like line 1944 which is why I suspect a copy-paste issue. Yes, the orthonormal 3x3 case the the transpose is the inverse so it's correct on line 1944. But the usage in line 1980 the matrix is 4x4 and includes the spacing on the diagonal and translation to the origin as the last column so the transpose would in general not be the same as the inverse (except in the special case of an identity matrix I guess).
@pieper This is going to take a little longer. NONE OF THE TEST SUITES TOUCHES THIS CODE! I need to make a test case to trigger this code.