ITK's SpatialOrientation and OrientImageFilter have opposite definitions of the DICOM standard

[blowekamp]

The definitions of the coordinate terms in SpatialOrientationEnums is as follows:

Coordinate orientation codes have a place-value organization such that an ImageDimension-al sequence of subcodes says both which varies fastest through which varies slowest, but also which end of the frame of reference is considered zero for each of the coordinates. For example, 'RIP' means Right to Left varies fastest, then Inferior to Superior, and Posterior to Anterior varies the slowest.

This is the opposite of the definitions for the DICOM standard: https://www.slicer.org/wiki/Coordinate_systems

It is interpreted that the preferred way to describe coordinates is in the "TO" direction or the direction of an axes is increasing. See the following DICOM section: C.7.6.2.1.1

ITK is defined as having a LPS coordinate system. However the SpatialOrientation enums and the OrientImageFilters call the RAI coordinates. This is confusing.

UPDATED:

I propose moving itk::OrientImageFilter to the deprecated module. Users can using this filter by simply enabling this module as it will be available until least ITK 7.0 ( unplanned ).

The itk::SpatialOrientationEnums and itk::SpatialOrientationAdapters usage should be discouraged. Usage where appropriate will be marked with "ITK_FUTURE_LEGACY_REMOVE", which provides no clear time frame for removal.

The core improvement is the introduction of the "DICOMOrientation" class which contains enum classes to describe spatial orientation as interpreted from DICOM with ITK having "LPS" coordinate. The DICOMOrientation class include the functionality of the legacy SpatialOrientationAdaptor, but with the addition of interpreting string representations. Additionally, this new class provides compatibly with conversion from the legacy SpatialOrientationEnums.

The DICOMOrientImageFilter from the SimpleITKFilters remote module is introduced to the "filter user" front end api, to clarify the change to the new DICOM based coordinate descriptions.

[blowekamp]

@dzenanz @lassoan @thewtex Care to chime in on this proposal?

[dzenanz]

I like it, but there is a lot work there. We should strive to keep the functionality on the same level, including Python accessibility. Also, there are a few examples which should be updated to demonstrate the new recommended syntax and filter names. If you are willing to do this, I am all for it!

[blowekamp]

The DICOMOrientImageFilter is in the SimpleITKFilters remote module and is available to pip install with "itk_simpleitkfilters". So please check it out and see if it is already on the same level of accessibility. I am not sure there is much work to do with updating the filter.

Additionally, I don't see examples using the filter. Perhaps because of this discrepancy in the orientation terms.

[dzenanz]

How is it expected to interact with SpatialOrientationAdapter? What about this test? I assume that DICOMOrientation would replace SpatialOrientation? Namely, as long as we can accomplish things like these using the new filters, we are fine.

[thewtex]

@blowekamp sounds good to me :+1:

[lassoan]

I've found the old names very confusing, so this change is very welcome.

To reduce the scope of the change, we could keep the current filters and just add new enum values and method names. For example, ...ITK_COORDINATE_ORIENTATION_RAI=ITK_COORDINATE_ORIENTATION_DICOM_LPS=1...

[blowekamp]

I've found the old names very confusing, so this change is very welcome.

To reduce the scope of the change, we could keep the current filters and just add new enum values and method names. For example, ...ITK_COORDINATE_ORIENTATION_RAI=ITK_COORDINATE_ORIENTATION_DICOM_LPS=1...

I think having classes with both will be rather confusing, and error prone, not to mention more work than the replacement with new classes. Additionally, what is the behavior of the string codes e.g. "RAI"? Runtime legacy options?

[blowekamp]

@dzenanz Good points. It has been a while since I looked a these details.

How is it expected to interact with SpatialOrientationAdapter?

So it looks like there are some changes with how the enums are handled, and deserves a review. The new DICOMOrientation class hold the enums declarations. And instance of DICOMOrientation has a OrientationEnum, and interfaces to translate between the enum, String, and Direction matrix via constructors and accessors.

Like this:

In [10]: lps = itk.DICOMOrientation("LPS")

In [11]: lps.GetAsDirection()
Out[11]: itkMatrixD33 ([[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]])

In [12]: lps.GetAsOrientation()
Out[12]: 591107

In [13]: lps.GetAsString()
Out[13]: 'LPS'

In [14]: rai = itk.DICOMOrientation(itk.DICOMOrientation.OrientationEnum_RAI)

In [15]: rai.GetAsDirection()
Out[15]: itkMatrixD33 ([[-1.0, 0.0, 0.0], [0.0, -1.0, 0.0], [0.0, 0.0, -1.0]])

What about this test?

That can be updated with the new interface above. I have honestly lost track of how Enums should be access and created in C++ ITK much less how SWIG is wrapping the current a legacy enums. This is a good opportunity to review and ensure they are optimized and correct.

I assume that DICOMOrientation would replace SpatialOrientation? Namely, as long as we can accomplish things like these using the new filters, we are fine.

As demo'd above I the new "DICOMOrentation" class should provide a consistent interface for switching between the tree orientation representation.

[lassoan]

When string code is get/set with the new methods like Get/SetDICOMOrientation(string code) then the DICOM definition is used.

Having the old deprecated methods and enum values is less clean now. However, it is a very small and simple development task for ITK developers and very smooth transition for everyone. The deprecated methods and enums can be removed in a couple of years.

Introducing new classes is acceptable, too, it would be immediately more clear solution, it is just signifcantly more work.

[blowekamp]

When string code is get/set with the new methods like Get/SetDICOMOrientation(string code) then the DICOM definition is used.

Having the old deprecated methods and enum values is less clean now. However, it is a very small and simple development task for ITK developers and very smooth transition for everyone. The deprecated methods and enums can be removed in a couple of years.

Introducing new classes is acceptable, too, it would be immediately more clear solution, it is just signifcantly more work.

Let me clarify my proposal and how it will work with compatibility of current usage of the backwards OrientImageFilter's direction labels.

My proposal is to move the current classes and enums to the ITK's Compatibility/Deprecated module as is. This means that for ITK 6.x they can continued to be used without change (maybe a flag will be needed to suppress a warning). At your convince over the next 5 years or so you can migrate to the new DICOMOrientation and DICOMOrientImageFilter, and fix the backwards labels being used with the deprecated class. When ITK 7.0 is released these deprecated classes likely will be removed or place in an unmaintained remote module or some such. I believe this is a smooth, clean and reasonable transition process.

The new classes already exist and are wrapped for ITK Python an are in pypi for ITK 5.4 as show above it further evaluation of the new interface or usage is needed.

https://github.com/InsightSoftwareConsortium/ITKSimpleITKFilters/blob/master/include/itkDICOMOrientation.h https://github.com/InsightSoftwareConsortium/ITKSimpleITKFilters/blob/master/include/itkDICOMOrientImageFilter.h

[blowekamp]

Having the old deprecated methods and enum values is less clean now. However, it is a very small and simple development task for ITK developers and very smooth transition for everyone. The deprecated methods and enums can be removed in a couple of years.

@lassoan I added a constructor to the new DICOMOrientation which takes the legacy SpatialOrientation enums blocked off with "ITK_FUTURE_LEGACY_REMOVE". The proposed DICOMOrientation class/enum has the methods to convert between representation instead of a separate adaptor class. See #4791 for details. Hopefully this will help with a smooth transition.

[blowekamp]

The new classes proposed in #4791 provide the following unambiguous way to describe a coordinate system:

-       itk::SpatialOrientationEnums::ValidCoordinateOrientations::ITK_COORDINATE_ORIENTATION_RSP;
+         hdr->coordinateOrientation = DICOMOrientation( DICOMOrientation::CoordinateEnum::RightToLeft,
                                                       DICOMOrientation::CoordinateEnum::SuperiorToInferior,
                                                       DICOMOrientation::CoordinateEnum::PosteriorToAnterior);

[lassoan]

The proposed implementation looks good to me. It is clean and transition should not be difficult.

Maybe DICOMOrientImageFilter name could a bit more generic, to allow it to be used for other kinds of coordinate systems. For example, it could be AntomicalOrientImageFilter or something even more generic.

[blowekamp]

@lassoan @seanm @issakomi The propose DICOMOrientationImageFilter and DICOMOrientations appear to have been a straw man and a catalyst to come up with some unambiguous annotations and terminology to describe orientations.

There are two important ITK components being addressed here:

• the "enum" or class for representing orientations itk::SpacialOrientationEnums
• the filter used to help convert images between them itk::OrientImageFilter

Maybe DICOMOrientImageFilter name could a bit more generic, to allow it to be used for other kinds of coordinate systems. For example, it could be AntomicalOrientImageFilter or something even more generic.

Yes, "AnatomicalOrientation" is much clearer, the DICOM notation appears to be in accurate and only lossy inferred from the DICOM specification. Let us apply that to the enum/class first and rename "DICOMOrientation" to "AnotomicalOrientation".

It is also clear to me the most unambiguous way to describe these anatomical orientations is something like the following:

coordinateOrientation = AnatomicalOrientation( AnatomicalOrientation::CoordinateEnum::RightToLeft,
                                                       AnatomicalOrientation::CoordinateEnum::SuperiorToInferior,
                                                       AnatomicalOrientation::CoordinateEnum::PosteriorToAnterior);

Which may benefit from some type aliases.

Also these 3 characters enums like "LPS" likely should have a type/space to describe them as "ToCoordinates" and a separate enums with "FromCoordinates".

The propose DICOMOrentation->AnatomicalOrientation class has conversions between type and representations so this might be a smoother transition than initially proposed requiring the disruptive replaced of OrientImageFilter with DICOMOrientImageFilter.

I plan to work on a PR with addition of AnatomicalOrientation and the usage of through ITK as done with the current PR and the DICOMOrientation class. This will not include the introduction of DICOMOrientationImageFilter etc..

[dzenanz]

Going forward, I should have more time to devote to code reviews, and I should be able to provide early feedback on this effort - should you want it, of course.

[issakomi]

It is rather easy to improve the ambiguity with "from" and "to" notations without deprecation of itkSpatial classes and OrientImageFilter, s. this post

Again, there are no such 3-letter enums and left-handed orientations in DICOM. The name is very misleading. A user may think it is applicable only to DICOM. Yes, "to" notations are more common and there are another orientation codes in DICOM e.g. "L\F": x to left, y to feet, specially for IODs without IPP/IOP, but unrelated to ITK logic.

Please don't remove itkSpatial classes and OrientImageFilter, just add new enums with clear names, both "from" and "to", it is easy.

[thewtex]

xref the OME-Zarr anatomical orientation proposal: https://github.com/ome/ngff/pull/253 , which is would be nice to use with this.

Something worth noting that @lassoan pointed out in the discussion is that DICOM uses Head / Foot, H/F instead of Superior / Inferior, S/I:

https://dicom.nema.org/medical/dicom/current/output/chtml/part03/sect_C.7.6.html#sect_C.7.6.1.1.1

[issakomi]

It is really worrying that ITK seems to break this stuff. From practical point of view. There is nothing wrong with it, it is IMO the best implementation. Just clear the ambiguity with enums, please.

[blowekamp]

xref the OME-Zarr anatomical orientation proposal: ome/ngff#253 , which is would be nice to use with this.

https://dicom.nema.org/medical/dicom/current/output/chtml/part03/sect_C.7.6.html#sect_C.7.6.1.1.1

What would compatibility or support for that proposal look like for an orientation representation?

[thewtex]

What would compatibility or support for that proposal look like for an orientation representation?

There is one-to-one mapping. Even better, there is direct mapping.

coordinateOrientation = AnatomicalOrientation( AnatomicalOrientation::CoordinateEnum::RightToLeft,
                                                       AnatomicalOrientation::CoordinateEnum::SuperiorToInferior,
                                                       AnatomicalOrientation::CoordinateEnum::PosteriorToAnterior);

This sounds perfect.

[lassoan]

Please don't remove itkSpatial classes and OrientImageFilter, just add new enums with clear names, both "from" and "to", it is easy.

I agree - when nothing is broken, no new features are added, then changes (even if small ones) are generally not welcome by ITK users. That said, the described deprecation plan (5 years available for application developers to switch to the new classes, detailed documentation will be provided, helpful deprecation messages will tell users they need to update their code and how) should minimize user frustration.

The only thing that bothers me a very little about the current rather heavyweight solution is that this effort could have been spent on more useful, more impactful ITK-related work. For example, reducing the issue and pull requests backlogs, improving OME NGFF file format support, ITK transforms (automatic on-the-fly inversion, extrapolation to entire 3D domain), harmonizing ITK and VTK build systems (keep best parts of both), etc. might have been more useful for the community.

[issakomi]

BTW, individual enums can be deprecated like this:

enum class MyEnums
{
    X = 1,
    Y [[deprecated]] = X
};

int main()
{
    int x = static_cast<int>(MyEnums::Y);
    return 0;
}

pseudo-code

{
  ITK_ORIENTATION_NOTATION_FROM_RAI = ... // old def for ITK_ORIENTATION_RAI
  ITK_ORIENTATION_NOTATION_TO_LPS = ITK_ORIENTATION_NOTATION_FROM_RAI,
...
#ifndef LEGACY_REMOVE
  ITK_ORIENTATION_RAI [[deprecated]] = ITK_ORIENTATION_NOTATION_FROM_RAI,
...
#endif
}