Add AnatomicalOrientation class #4804

blowekamp · 2024-08-14T15:10:05Z

This PR is a WIP, and more details will be added here when ready for discussion and review.

See #4788 for this issue and discussion of these changes.

dzenanz

Looks good so far!

blowekamp · 2024-08-15T13:55:45Z

Please move this conversation about MINC to #147 or another place on topic.

Modules/Core/Common/include/itkAnatomicalOrientation.h

Modules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx

Modules/Core/Common/test/itkAnatomicalOrientationGTest.cxx

blowekamp · 2024-08-16T18:42:28Z

@dzenanz Almost there. There is currently both the ToEnum, and FromEnum. The class "AnatomicalOrientation" can be seen to hold the representation of the orientation, and provide conversion with the Enum's, direction matrix, and perhaps the preferred handling it as 3 components.

There is still the ToEnum string representation left over from the original DICOMOrientation class. That is AnatomicalOreination("LPS") construction and AnatomicalOrientation(OE::LPS).ToString() == "LPS". Anatomicalorientation::operator<< produce something like "left-to-right, anterior-to-posterior, inferior-to-superior" now, which is unambiguous. Perhaps the string character tripplets should just be removed? However the string representation is convent with a Python interface.

dzenanz · 2024-08-16T19:06:20Z

AnatomicalOreination("LPS") construction and AnatomicalOrientation(OE::LPS).ToString() == "LPS"

I would like to have this. This convenience did not exist within ITK proper before, so there is no need for some sort of backwards compatibility. I wonder whether we should have a method to convert "To" string from a "From" string and vice versa. E.g. if an application has "RAI" string as a "From" code, it can call something like AnatomicalOrientation::ConvertToFrom("RAI") and get "LPS". Such method would be symmetric, as the same operation would be needed to go from "From" to "To", and from "To" to "From" convention.

blowekamp · 2024-08-16T19:38:54Z

So your suggestion is to keep the ambiguous "ToEnum" character representation, and just add a method to convert To/From character representations?

The concern here is really when AnatomicalOrientation("RIA") would be used, it is not clear if the string is being interpreted as a ToEnum or a FromEnum.

dzenanz · 2024-08-16T19:53:02Z

If you want, you can remove the string constructor. Is there another similarly convenient way to initialize it? Something like:

auto identityAO = AnatomicalOrientation::ToEnum("LPS");
AnatomicalOrientation ao(identityAO);

dzenanz · 2024-08-16T19:54:09Z

More generally,:

std::string toCode = "LPS";
auto aoEnum = AnatomicalOrientation::ToEnum(toCode);
AnatomicalOrientation ao(aoEnum);

Modules/Core/Common/include/itkAnatomicalOrientation.h

dzenanz · 2024-08-21T15:36:36Z

What is the status of this PR? I would like it to be finished/merged before it becomes stale, as PR revival usually involves extra effort.

Modules/Filtering/ImageGrid/include/itkOrientImageFilter.h

blowekamp · 2024-09-03T16:49:49Z

@dzenanz Shall we have the SpatialOrientation filter changes be in another PR?

dzenanz · 2024-09-03T17:24:09Z

I am fine with that. And that might even be better, to not hold up this PR.

Modules/Core/Common/include/itkAnatomicalOrientation.h

thewtex

🍾

blowekamp · 2024-10-22T20:11:10Z

I'm planning on merging this tomorrow unless there are further comments.

N-Dekker · 2024-10-23T12:13:30Z

@blowekamp Shall I still have a look before the merge?

blowekamp · 2024-10-23T12:23:06Z

@blowekamp Shall I still have a look before the merge?

yes please.

N-Dekker · 2024-10-23T13:55:13Z

Modules/Core/Common/src/itkAnatomicalOrientation.cxx

+AnatomicalOrientation::PositiveEnum
+AnatomicalOrientation::ConvertDirectionToPositiveEnum(const DirectionType & dir)
+{
+  // NOTE: This method was based off of itk::SpatialObjectAdaptor::FromDirectionCosines
+  // but it is DIFFERENT in the meaning of direction in terms of sign-ness.
+  CoordinateEnum terms[3] = { CoordinateEnum::UNKNOWN, CoordinateEnum::UNKNOWN, CoordinateEnum::UNKNOWN };
+
+  std::multimap<double, std::pair<unsigned, unsigned>> value_to_idx;
+  for (unsigned int c = 0; c < 3; ++c)
+  {
+    for (unsigned int r = 0; r < 3; ++r)
+    {
+      value_to_idx.emplace(std::abs(dir[c][r]), std::make_pair(c, r));
+    }
+  }


ConvertDirectionToPositiveEnum is hard for me to understand. I hope the other reviewers do understand it better than I do 😃

The math or the general input and output of the method?

So you are creating this multi-map, in order to find the direction matrix element that has the highest absolute value, right? (Just for my understanding -- this is not a change request.)

Yes, that appears to be what is going on. As the comment says it was mostly a copy of the existing working method in itk::SpatialObjectAdaptor::FromDirectionCosines

OK, thanks, and the math itself? How does it work? Is it a known approach?

It is a tested algorithm that worked for all the test cases I put it through. It explicitly does what a "smarter" and incorrect algorithm did previously:
8801247

dzenanz · 2024-10-23T18:12:42Z

🎆 🎊 🥳

N-Dekker · 2024-10-24T15:50:24Z

@blowekamp There appear some compile errors at the CI (master branch), if you know how to fix them, please do 😃 At https://open.cdash.org/viewBuildError.php?buildid=9981860


/Users/builder/externalModules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx:713:61: error: no member named 'FromEnum' in 'itk::AnatomicalOrientation'
  AnatomicalOrientation coord_orient(AnatomicalOrientation::FromEnum::RSA);
                                     ~~~~~~~~~~~~~~~~~~~~~~~^
[CTest: warning matched] /Users/builder/externalModules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx:713:37: warning: parentheses were disambiguated as a function declaration [-Wvexing-parse]
  AnatomicalOrientation coord_orient(AnatomicalOrientation::FromEnum::RSA);
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[CTest: warning matched] /Users/builder/externalModules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx:713:38: note: add a pair of parentheses to declare a variable
  AnatomicalOrientation coord_orient(AnatomicalOrientation::FromEnum::RSA);
                                     ^

blowekamp · 2024-10-24T18:27:35Z

Follow up: #4899

jhlegarreta · 2024-10-27T18:21:04Z

Seems like there's a warning in the ITK.Windows.Python Azure build introduced by this PR:

D:\a\1\s-build\Wrapping\Modules\ITKCommon\itkAnatomicalOrientationPython.cpp(3971):
 warning C4996: 'itk::AnatomicalOrientation::AnatomicalOrientation':
 Use the AnatomicalOrientation::FromEnum type instead.

e.g.
https://dev.azure.com/itkrobotwindowpython/ITK.Windows.Python/_build/results?buildId=4113&view=logs&j=2d2b3007-3c5c-5840-9bb0-2b1ea49925f3&t=77aad734-2057-5694-9ae2-ee1f5f26eae8&l=4288

Visible in the dashboard as well:
https://open.cdash.org/viewBuildError.php?type=1&buildid=9988068

Not sure how the below block could be handled better to avoid the warning:
https://github.com/InsightSoftwareConsortium/ITK/pull/4804/files#diff-0271cfd0418afac76ae9a99eb3c59923eac04cdfc032bbf51c3bc59c88ca1c81R441-R443

blowekamp · 2024-10-27T21:05:07Z

Not sure how the below block could be handled better to avoid the warning: https://github.com/InsightSoftwareConsortium/ITK/pull/4804/files#diff-0271cfd0418afac76ae9a99eb3c59923eac04cdfc032bbf51c3bc59c88ca1c81R441-R443

Neither am I. Is the build/wrapping defining ITK_LEGACY_SILENT when compiling?

N-Dekker · 2024-10-27T21:13:45Z

@blowekamp Please note that the warning still uses the term FromEnum. Didn't you replace that by Positive/Negative Enum?

blowekamp · 2024-10-27T21:25:51Z

Please note that the warning still uses the term FromEnum. Didn't you replace that by Positive/Negative Enum?

Yes, good catch the text in the warning should be updated as you recommend. But the issue is that the warning should not be occurring.

blowekamp · 2024-10-27T21:28:52Z

Perhaps the bigger problem on the nightly build:

/Users/builder/external[Modules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx:721](https://github.com/InsightSoftwareConsortium/ITK/blob/0883d0c3b7967b9319ad9ac3a3d15fae576e382c/Modules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx#L721):20: error: no viable overloaded '='
      coord_orient = SpatialOrientationEnums::ValidCoordinateOrientations::ITK_COORDINATE_ORIENTATION_RAI;
      ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[CTest: warning matched] /Users/builder/external[Modules/Core/Common/include/itkAnatomicalOrientation.h:53](https://github.com/InsightSoftwareConsortium/ITK/blob/0883d0c3b7967b9319ad9ac3a3d15fae576e382c/Modules/Core/Common/include/itkAnatomicalOrientation.h#L53):24: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'itk::SpatialOrientationEnums::ValidCoordinateOrientations' to 'const itk::AnatomicalOrientation' for 1st argument
class ITKCommon_EXPORT AnatomicalOrientation
                       ^

@seanm may be compiling with LEGACY_REMOVE enabled so this conversion function is removed.

dzenanz · 2024-10-28T12:46:31Z

Most of Ryzenator's and Corista's builds have ITK_LEGACY_REMOVE:BOOL=ON.

blowekamp · 2024-10-28T12:58:43Z

Most of Ryzenator's and Corista's builds have ITK_LEGACY_REMOVE:BOOL=ON.

But they don't have PhilipsREC module enabled?

dzenanz · 2024-10-28T13:02:24Z

But they don't have PhilipsREC module enabled?

Ah, that is a non-default module, which I indeed did not turn on.

dzenanz reviewed Aug 14, 2024

View reviewed changes

This comment was marked as off-topic.

Sign in to view

blowekamp force-pushed the add_anatomical_orientation branch from 2502620 to fb83a56 Compare August 15, 2024 12:44

This comment was marked as off-topic.

Sign in to view

blowekamp commented Aug 15, 2024

View reviewed changes

Modules/Core/Common/include/itkAnatomicalOrientation.h Outdated Show resolved Hide resolved

seanm reviewed Aug 15, 2024

View reviewed changes

Modules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx Outdated Show resolved Hide resolved

blowekamp force-pushed the add_anatomical_orientation branch from fb83a56 to 4042eb3 Compare August 15, 2024 21:06

blowekamp commented Aug 16, 2024

View reviewed changes

Modules/Core/Common/test/itkAnatomicalOrientationGTest.cxx Show resolved Hide resolved

blowekamp force-pushed the add_anatomical_orientation branch from 4042eb3 to 3e2ef5b Compare August 16, 2024 18:35

issakomi reviewed Aug 16, 2024

View reviewed changes

Modules/Core/Common/include/itkAnatomicalOrientation.h Outdated Show resolved Hide resolved

blowekamp force-pushed the add_anatomical_orientation branch from 3e2ef5b to 62a566b Compare September 3, 2024 16:05

github-actions bot added the area:Filtering Issues affecting the Filtering module label Sep 3, 2024

dzenanz reviewed Sep 3, 2024

View reviewed changes

Modules/Filtering/ImageGrid/include/itkOrientImageFilter.h Outdated Show resolved Hide resolved

blowekamp force-pushed the add_anatomical_orientation branch from 62a566b to 7192104 Compare September 3, 2024 18:00

github-actions bot removed the area:Filtering Issues affecting the Filtering module label Sep 3, 2024

blowekamp force-pushed the add_anatomical_orientation branch from b642f96 to 58576c4 Compare October 21, 2024 18:30

dzenanz reviewed Oct 21, 2024

View reviewed changes

Modules/Core/Common/include/itkAnatomicalOrientation.h Outdated Show resolved Hide resolved

N-Dekker reviewed Oct 21, 2024

View reviewed changes

Modules/Core/Common/include/itkAnatomicalOrientation.h Show resolved Hide resolved

DOC: Add documentation to the AnatomicalOrientation class

38d2397

blowekamp force-pushed the add_anatomical_orientation branch from 58576c4 to ded6b69 Compare October 21, 2024 19:17

N-Dekker reviewed Oct 21, 2024

View reviewed changes

Modules/Core/Common/include/itkAnatomicalOrientation.h Outdated Show resolved Hide resolved

ENH: Enable constexpr construciton of AnatomicalOrientation

1fb80ea

blowekamp force-pushed the add_anatomical_orientation branch from ded6b69 to 1fb80ea Compare October 22, 2024 00:25

blowekamp requested review from N-Dekker and thewtex October 22, 2024 10:00

dzenanz approved these changes Oct 22, 2024

View reviewed changes

thewtex approved these changes Oct 22, 2024

View reviewed changes

N-Dekker reviewed Oct 23, 2024

View reviewed changes

blowekamp merged commit 283ad56 into InsightSoftwareConsortium:master Oct 23, 2024
13 checks passed

blowekamp mentioned this pull request Oct 28, 2024

ENH: Add DICOMOrientation class from remote module #4791

Closed

7 tasks

blowekamp mentioned this pull request Jan 23, 2025

ITK SpatialOrientation is opposite of DICOM and ITK annotation. #1163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AnatomicalOrientation class #4804

Add AnatomicalOrientation class #4804

blowekamp commented Aug 14, 2024

dzenanz left a comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

blowekamp commented Aug 15, 2024

blowekamp commented Aug 16, 2024

dzenanz commented Aug 16, 2024

blowekamp commented Aug 16, 2024

dzenanz commented Aug 16, 2024

dzenanz commented Aug 16, 2024 •

edited

Loading

dzenanz commented Aug 21, 2024

blowekamp commented Sep 3, 2024

dzenanz commented Sep 3, 2024

thewtex left a comment

blowekamp commented Oct 22, 2024

N-Dekker commented Oct 23, 2024

blowekamp commented Oct 23, 2024

N-Dekker Oct 23, 2024

blowekamp Oct 23, 2024

N-Dekker Oct 23, 2024 •

edited

Loading

blowekamp Oct 23, 2024

N-Dekker Oct 23, 2024

blowekamp Oct 23, 2024

dzenanz commented Oct 23, 2024

N-Dekker commented Oct 24, 2024

blowekamp commented Oct 24, 2024

jhlegarreta commented Oct 27, 2024 •

edited

Loading

blowekamp commented Oct 27, 2024

N-Dekker commented Oct 27, 2024

blowekamp commented Oct 27, 2024

blowekamp commented Oct 27, 2024

dzenanz commented Oct 28, 2024

blowekamp commented Oct 28, 2024

dzenanz commented Oct 28, 2024

Add AnatomicalOrientation class #4804

Add AnatomicalOrientation class #4804

Conversation

blowekamp commented Aug 14, 2024

dzenanz left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

blowekamp commented Aug 15, 2024

blowekamp commented Aug 16, 2024

dzenanz commented Aug 16, 2024

blowekamp commented Aug 16, 2024

dzenanz commented Aug 16, 2024

dzenanz commented Aug 16, 2024 • edited Loading

dzenanz commented Aug 21, 2024

blowekamp commented Sep 3, 2024

dzenanz commented Sep 3, 2024

thewtex left a comment

Choose a reason for hiding this comment

blowekamp commented Oct 22, 2024

N-Dekker commented Oct 23, 2024

blowekamp commented Oct 23, 2024

N-Dekker Oct 23, 2024

Choose a reason for hiding this comment

blowekamp Oct 23, 2024

Choose a reason for hiding this comment

N-Dekker Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

blowekamp Oct 23, 2024

Choose a reason for hiding this comment

N-Dekker Oct 23, 2024

Choose a reason for hiding this comment

blowekamp Oct 23, 2024

Choose a reason for hiding this comment

dzenanz commented Oct 23, 2024

N-Dekker commented Oct 24, 2024

blowekamp commented Oct 24, 2024

jhlegarreta commented Oct 27, 2024 • edited Loading

blowekamp commented Oct 27, 2024

N-Dekker commented Oct 27, 2024

blowekamp commented Oct 27, 2024

blowekamp commented Oct 27, 2024

dzenanz commented Oct 28, 2024

blowekamp commented Oct 28, 2024

dzenanz commented Oct 28, 2024

dzenanz commented Aug 16, 2024 •

edited

Loading

N-Dekker Oct 23, 2024 •

edited

Loading

jhlegarreta commented Oct 27, 2024 •

edited

Loading