InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.47k stars 686 forks source link

Error in nrrdImageIO #5207

Closed rbakker closed 1 week ago

rbakker commented 1 week ago

Description

In nrrdImageIO, line 490, it reads: """ case nrrdSpaceLeftAnteriorSuperior: spaceDir[0] *= -1; // R -> L iFlipFactors[0] = -1; break; case nrrdSpaceLeftPosteriorSuperior: // no change needed break; """

But if the standard space in ITK is LeftPosteriorSuperior, then to go to LeftAnteriorSuperior a change is needed in spaceDir[1], not spaceDir[0]. So the code should be:

""" case nrrdSpaceLeftAnteriorSuperior: spaceDir[1] *= -1; // A -> P iFlipFactors[1] = -1; break; case nrrdSpaceLeftPosteriorSuperior: // no change needed break; """

The issue occurs twice in the same file, also on nrrdImageIO, line 539.

Steps to Reproduce

Have not tested this bug, it is already obvious from the code.

Expected behavior

Actual behavior

Reproducibility

Versions

Environment

Additional Information

github-actions[bot] commented 1 week ago

Thank you for contributing an issue! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜 Also, please check existing open issues and consider discussion on the ITK Discourse. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the issue and comment on it.

dzenanz commented 1 week ago

@kindlmann This seems obvious. Do you agree? I assume we are not frequently running into nrrdSpaceLeftAnteriorSuperior case, so this managed to survive for so many years.

jhlegarreta commented 1 week ago

@rbakker Thanks for reporting this. Can we please add a test that demonstrates the issue and so that issues of this kind do not go unnoticed ever again?

kindlmann commented 1 week ago

I agree that the code as is looks wrong, and I agree with the fix.

The NRRD file format provides for world-space being nrrdSpaceLeftAnteriorSuperior as a shout-out to old (pre-NiFTI) Analyze files.

I don't have any such files handy now, and to be honest I don't remember testing that particular code on such files when I wrote it so long ago.

Are there any old Analyze datasets (containing known anatomy) in their original form, rather than datasets generated by modern tools, that could be used to authentically test this?

issakomi commented 1 week ago

The origin can be vtkTeemNRRDReader.cxx, L488-L491, there it is correct for the Slicer's RAS space

            case nrrdSpaceLeftAnteriorSuperior:
              spaceDir[0] *= -1;   // L -> R
              this->NRRDWorldToRasMatrix->SetElement(0, 0, -1);
              break;
kindlmann commented 1 week ago

@rbakker can you link here to the "nrrdImageIO, line 490" file so I can see the "blame"/commit history?

rbakker commented 1 week ago

I now put links to nrrdImageIO in the original post.

kindlmann commented 1 week ago

Ok thanks. Hmmm - the lines in question are "blamed" on @blowekamp from a change in 2012, about a memory management issue ("Fix leak in NrrdIO when file doesn't exit") but I don't think that consequentially changed the behavior of the lines handling nrrdSpaceLeftAnteriorSuperior. How do we track down their real origin? (I'm guessing it was me, but want to be sure).

Still, the right way to test this is to find some vintage Analyze files and watch how ITK's NrrdIO is handling it.

dzenanz commented 1 week ago

Here is how to follow blame chain on GitHub:

Image

After two steps we end here: https://github.com/InsightSoftwareConsortium/ITK/commit/15372e8c17ef2344d47767d47cc27c5fe9c6c17a#diff-d449da6e71973daac03dff7ecc0fb001eebf79cd47c51a580c77e98e302c4dccR480 which means that the original contribution from 2005 already had the bug.

kindlmann commented 1 week ago

Okay. Your last link is about Commit 15372e8, which seems to be from Bill Lorensen. Sigh. I can still hear his laugh.

I will always be grateful for how Bill pushed me to think hard about how to represent image orientation in NRRD, which is what led to the creation of the "space" field that gives anatomical semantics to the world-space, and the "space direction" and "space origin" fields that, in that world-space, give the per axis inter-sample offsets, and the location of the first sample. That allowed NRRD to cleanly separate index-space from world-space, and I've been continuing to rely on that every day since then.

In any case, there does seem to be a bug. @rbakker what data or process led to you running into this bug? Did it start with a vintage Analyze file?

rbakker commented 1 week ago

I used SimpleITK to convert a Nifti1 file to NRRD and then Three.js NRRDImageLoader to display the volume. This gave me an orientation error and made me look in both the ITK source code and Three.js addon source code. The NRRD file produced by ITK had space=LeftPosteriorSuperior, and modified the Nifti1 affine matrix (in RAS orientation) correctly. So the bug I was looking for was not in ITK. But then I saw the code for LeftAnteriorSuperior and that is obviously wrong. But I do not have a particular file to test it.

dzenanz commented 1 week ago

5224 just fixes the bug, it does not add a test. Addition of a test can be a separate PR.

kindlmann commented 1 week ago

At the risk of needlessly prolonging this @rbakker: if your Nifti1 file had orientation specified in the RAS world-space (as you mention above), then why does that implicate a line in ITK for handling LAS world-space?

Can you explain in more detail what you think is the chronology of the different files, pieces of software, and world-spaces involved? What was actually touching on LAS space?

I know from sad experience that this kind of coordinate errors are sometimes fixed by adding a negation until things look right, but I also know that it only adds mystery and future frustration to things that take time to understand thoroughly. This isn't really my code to be concerned about, but now that it has my attention, and maybe just for posterity, I want to be better convinced that this modification actually a fix for the issue that started this.

blowekamp commented 1 week ago

Related to this issue is the clear communication of the anatomical orientation in an unambiguous fashion. This new ITK class may be of interest: https://docs.itk.org/projects/doxygen/en/latest/classitk_1_1AnatomicalOrientation.html

dzenanz commented 1 week ago

why does that implicate a line in ITK for handling LAS world-space

It doesn't, it is just near. And when he saw the bug, he reported.

kindlmann commented 1 week ago

Uh. Again, not my codebase, but I would not change something that my own experience/tests are not directly touching on. We don't know what other tools or workflows might be using RAS space, and relying on the current functionality.

dzenanz commented 1 week ago

But this is the rather rare LAS space that is being impacted.

rbakker commented 1 week ago

why does that implicate a line in ITK for handling LAS world-space

It doesn't, it is just near. And when he saw the bug, he reported.

This is right, I just saw this bug in the code when I looked for something else nearby.