Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Add DICOMOrientation class from remote module #4791

Conversation

blowekamp
Copy link
Member

@blowekamp blowekamp commented Jul 31, 2024

The class provides anatomical orientation based enums for describing coordinate systems. In ITK the physical coordinate systems is the same as the DICOM standard. DICOM has the following specification:

If Anatomical Orientation Type (0010,2210) is absent or has a value of BIPED, the x-axis is increasing to the left hand side of the patient. The y-axis is increasing to the posterior side of the patient. The z-axis is increasing toward the head of the patient.

Which describes the direction of increasing. The above coordinate system is described as "LPS" with this enumeration and corresponds to an ITK image with an identity direction cosine matrix.

See also #4788, #4788

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.

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Jul 31, 2024
@github-actions github-actions bot added the area:Filtering Issues affecting the Filtering module label Jul 31, 2024
@blowekamp blowekamp mentioned this pull request Jul 31, 2024
7 tasks
The class provides anatomical orientation based enums for describing
coordinate systems. In ITK the physical coordinate systems is the same
as the DICOM standard. DICOM has the following specification:

If Anatomical Orientation Type (0010,2210) is absent or has a value of
BIPED, the x-axis is increasing to the left hand side of the
patient. The y-axis is increasing to the posterior side of the
patient. The z-axis is increasing toward the head of the patient.

Which describes the direction of increasing. The above coordidate
system is described as "LPS" with this enumeration and corresponds to
an ITK image with an identity direction cosine matrix.
This is a replacement for the OrientImageFilter, which used the
DICOMOrientation enumerations and conventions for describing the
patient orientation by the increasing anaomical axis.
Ensure matching byte codes between DICOM and legacy coordinate enums
which have the same direction cosine interpretation.

Add unambiguous CoordinateEnum duplication of the form LeftToRight.

Add method to DICOMOrientationImageFilter which accepts
DICOMOrientation object, which can be implicitly converted from legacy
coordinate enums.
Replace usage of SpatialOrientationAdaptor with DICOMOrientation and
unambiguous FROMToTO CoordinateEnums in GE related ImageIO classes.
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far.

switch (this->m_NiftiImage->analyze75_orient)
{
case a75_transverse_unflipped:
orient = SpatialOrientationEnums::ValidCoordinateOrientations::ITK_COORDINATE_ORIENTATION_RPI;
orient = DICOMOrientation(DICOMOrientation::CoordinateEnum::RightToLeft,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these long and and long-ish statements, it might make sense to using CE = DICOMOrientation::CoordinateEnum, then use CE::RightToLeft, CE::PosteriorToAnterior, CE::InferiorToSuperior on a single line. This applies to many places in the code, including previous commits.

@dzenanz
Copy link
Member

dzenanz commented Aug 6, 2024

One warning, Modules/IO/GE/src/itkGE5ImageIO.cxx:191:44: warning: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'struct GEImageHeader'; use assignment or value-initialization instead [-Wclass-memaccess]

@issakomi
Copy link
Member

issakomi commented Aug 7, 2024

In ITK the physical coordinate systems is the same as the DICOM standard.

It is not exactly the same, 1/2 of ITK orientations are not defined in DICOM.
DICOM defines only right-handed orientations. "Image Orientation (Patient) Attribute" defines "x" and "y" (6 values), "z" is right-handed cross-product.

The Patient-Based Coordinate System is a right handed system, i.e., the vector cross product of a unit vector along the positive x-axis and a unit vector along the positive y-axis is equal to a unit vector along the positive z-axis.

ITK defines "x", "y" and "z", so e.g. RAI-Code (from) LPS

-1  0  0
 0 -1  0
 0  0 -1

exists in ITK and is not applicable to DICOM definition.

@issakomi
Copy link
Member

issakomi commented Aug 7, 2024

In fact I also think that something should be done with ITK's "from" orientation codes, it seems that surprisingly many users don't understand them at all. But how should they if there is no clear documentation and, even worse, at many places there are already "to" notations.
May be e.g. ITK_COORDINATE_ORIENTATION_RAI could be renamed to ITK_COORDINATE_ORIENTATION_NOTATION_FROM_RAI and be equal to ITK_COORDINATE_ORIENTATION_NOTATION_TO_LPS or something like that.
It will allow preserve AnatomicalOrientation field of Meta images (document properly!). Also the same for some methods returning orientation codes (clarify "to" and "from" in the name). And no need to kill OrientImageFilter and itkSpatial classes for the sake of "DICOM enums" which don't exist in DICOM.

Addresses:

warning: 'void* memset(void*, int, size_t)' clearing an object of
non-trivial type 'struct GEImageHeader'; use assignment or
value-initialization instead [-Wclass-memaccess]
@@ -1758,7 +1758,9 @@ set(ITKCommonGTests
itkWeakPointerGTest.cxx
itkCommonTypeTraitsGTest.cxx
itkMetaDataDictionaryGTest.cxx
itkSpatialOrientationAdaptorGTest.cxx)
itkSpatialOrientationAdaptorGTest.cxx
itkDICOMOrientionGTest.cxx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
itkDICOMOrientionGTest.cxx
itkDICOMOrientationGTest.cxx

@@ -715,6 +715,7 @@ orthonormality
opencl
openjpeg
oper
osterior
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is word does not belong to the dictionary, because it should be rejected. It would be better to avoid using this word in the source code or disable spellchecking at the single place where this word is intentionally used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a misspelling of posterior.

* \f[
* LPS = \begin{Bmatrix}
* from\ right\ to\ \textbf{L}eft \\
* from\ anterior\ towards\ \textbf{P}osterior \\
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dzenanz It was added to the dictionary due to the mark up of P.

@blowekamp
Copy link
Member Author

The work has evolved into another PR. I didn't expect the continued comments on this WIP, some were applied to the other PR still under development.

@seanm
Copy link
Contributor

seanm commented Aug 15, 2024

The work has evolved into another PR. I didn't expect the continued comments on this WIP, some were applied to the other PR still under development.

So wait, this one is dead? Should we only look at #4788 ? I'm confused with all these different PRs...

@blowekamp
Copy link
Member Author

So wait, this one is dead? Should we only look at #4788 ? I'm confused with all these different PRs...

The original issue #4788 is the discussion. I have not yet directly asked for feedback on the other PR, its still approaching being finished and incorporating the feed back I have heard.

There was strong opposition for using the specific "DICOM" term ( and apparently overly specific/inappropriate association), and the opposition to replacing the OritenImageFilter class, a new PR with a updated title, and an updates description seemed appropriate to help reset everyone for the updated goals.

@dzenanz
Copy link
Member

dzenanz commented Oct 28, 2024

Should this PR be closed now?

@blowekamp
Copy link
Member Author

See PR #4804 and #4895

@blowekamp blowekamp closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Python wrapping Python bindings for a class type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants