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.41k stars 663 forks source link

Anatomical orientation in MetaImages ignored in ITK #1017

Closed dzenanz closed 3 months ago

dzenanz commented 5 years ago

Description

AnatomicalOrientation tag in MetaImages is ignored. LPS is always assumed.

Steps to Reproduce

Have a .mha image with AnatomicalOrientation tag. Change the orientation e.g. from RAS to RAI. The image should be flipped after this change, but it isn't.

Additional Information

Direction cosines are read, but AnatomicalOrientation is not read, it is always assumed to be LPS.

What should be done: read AnatomicalOrientation, and if it is different from LPS apply an appropriate correction matrix to direction cosines.

Submitted by: @agirault.

agirault commented 5 years ago

Also:

ensure to handle the case where the acronym is invalid in the reader
update the Writer which currently infers the anatomical orientation from the direction cosines, to instead always write LPS, since that is the coordinate system associated with the direction matrix

issakomi commented 5 years ago

It is worrying. I tested in the past all possible orientations with MetaImages read/write and they looked correct for me. IMHO, the direction matrix is fully sufficient. I calculate orientation code (RAI, ASL, etc.) from the final image myself using spatial adapter (and have own functions too). Everything was fine and results are were consistent with anatomical orientation code in the file. Even more - i fully trusted MetaImage format and used files as reference. If there is a bug and orientation is broken (i don't think so) - it were epic fail and i'll have to notify customers and i don't think they will ever use my software or highly likely anything based on ITK. Here are MetaImage files i used as reference, all orientations, with right/left marker burned in: Orientation test images If something is wrong with, it were catastrophic.

Edit

looked again and i think it is false alarm, correct orientation is written. There are matrix and orientation code, i guess that reader takes matrix, writer writes both. So if you manually change orientation code and makes it inconsistent with the matrix - you break files, but any format can be broken.

Edit 2

Direction cosines are read, but AnatomicalOrientation is not read, it is always assumed to be LPS.

Default coordinate system is LPS and this tag doesn't change it. Matrix is read and images get correct orientation. This orientation code is not intended to change default coordinate system. Identity matrix is LPS (RAI in ITK's terminology). Others are what spatial adapter returns, e.g. "TransformMatrix = 0 1 0 0 0 -1 -1 0 0" is ASL, there are 48 possible orientation codes and it is working very well, IMHO. This is probably the reason for the issue - you have assumed this orientation code changes default coordinate system, no, it doesn't. If you want new functionality - to change default coordinate system - you must introduce new tag, otherwise backward compatibility will be hopeless broken.

What should be done: read AnatomicalOrientation, and if it is different from LPS apply an appropriate correction matrix to direction cosines.

update the Writer which currently infers the anatomical orientation from the direction cosines, to instead always write LPS, since that is the coordinate system associated with the direction matrix

Again, it is not default coordinate system, it is orientation code for current matrix.

Edit 3:

MetaIO Documentation

AnatomicalOrientation MET_STRING
Specify anatomic ordering of the axis. Use only [R|L] | [A|P] | [S|I] per axis. For example, if the
three letter code for (column index, row index, slice index is) ILP, then the origin is at the superior, 
right, anterior corner of the volume, and therefore the axes run from superior to inferior, from
right to left, from anterior to posterior.

dzenanz commented 5 years ago

Current treatment of AnatomicalOrientation tag seems to be purely for human convenience. @agirault recently came to the conclusion it should not be so, after discussion with @aylward and @thewtex. Perhaps reasoning and history behind this could be given here?

agirault commented 5 years ago

Thanks for the feedback @issakomi. This is a sensitive change so we will make sure to run our findings and suggestions by the ITK community on Discourse to find the best course of action and maintain backward compatibility.

Just for a summary:

the AnatomicalOrientation tag was created as independent information from the direction matrix and was meant to describe the coordinate system resulting from applying the transformation to IJK coordinates. It was added 15 years ago to ITK as a tag that would be read and written as metadata, which again was complementary to the direction matrix, not redundant.
14 years ago, changes were made in ITK were this tag was not read anymore, and it was computed from the direction matrix when being written. This made the tag redundant information to the direction matrix, which was not the intended use according to its creator (simple example: it doesn't make sense to infer the anatomical orientation if the transformation is not axis-aligned)
Even though this was not the original design, this has been in ITK for 14 years. We're aware that changing it would have big repercussion and we need to respect that.
On top of this, we need to better define what convention ITK uses to even define anatomical orientation: while DICOM/NRRD describes the axis at the tip of the XYZ vectors (L means +X=R->L), MetaIO and ITKSnap currently describe the axis as the origin of those vectors (L means +X=L->R). This was discussed a while back here.

aylward commented 5 years ago

Hi,

I don't know what MetaIO/ITK does, but according to the MetaIO documentation,

AnatomicalOrientation

MET_STRING
Specify anatomic ordering of the axis. Use only [R|L] | [A|P] | [S|I] per axis. For example, if the three letter code for (column index, row index, slice index is) ILP, then the origin is at the superior, right, anterior corner of the volume, and therefore the axes run from superior to inferior, from right to left, from anterior to posterior.

I believe that this definition of anatomic orientation is correct (i.e., consistent with DICOM). Please let me know if I am wrong.

Thanks, Stephen

On Fri, Jun 14, 2019 at 12:09 PM Alexis Girault notifications@github.com wrote:

Thanks for the feedback @issakomi https://github.com/issakomi. This is a sensitive change so we will make sure to run our findings and suggestions by the ITK community on Discourse to find the best course of action and maintain backward compatibility.

Just for a summary:

the AnatomicalOrientation tag was created as independent information from the direction matrix and was meant to describe the coordinate system resulting from applying the transformation to IJK coordinates. It was added 15 years ago to ITK as a tag that would be read and written as metadata, which again was complementary to the direction matrix, not redundant.

14 years ago, changes were made in ITK were this tag was not read anymore, and it was computed from the direction matrix when being written. This made the tag redundant information to the direction matrix, which was not the intended use according to its creator (simple example: it doesn't make sense to infer the anatomical orientation if the transformation is not axis-aligned)

Even though this was not the original design, this has been in ITK for 14 years. We're aware that changing it would have big repercussion and we need to respect that.

On top of this, we need to better define what convention ITK uses to even define anatomical orientation: while DICOM/NRRD describes the axis at the tip of the XYZ vectors (L means +X=R->L), MetaIO and ITKSnap currently describe the axis as the origin of those vectors (L means +X=L->R). This was discussed a while back here https://itk.org/Wiki/Proposals:Orientation#Current_ITK_Usage_and_sources_of_confusion .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJLY5TEX7YG6L4AQXCU3P2O7DJA5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXXHX2Y#issuecomment-502168555, or mute the thread https://github.com/notifications/unsubscribe-auth/AACEJL2SIUZ7C2FVAR2C6MTP2O7DJANCNFSM4HX5GQMA .

--

Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives, Kitware, Inc., North Carolina

Kitware: Advancing the frontiers of understanding by developing innovative open-source software platforms and integrating them into research, processes, and products.

dzenanz commented 5 years ago

But does this refer to ijk or XYZ origin and vectors? If it refers to ijk (index space), then direction matrix is redundant. If it refers to XYZ (physical space), then another (possible) flip is required to reconcile this with ITK convention that identity matrix corresponds to LPS.

issakomi commented 5 years ago

@agirault
Thank you for responce. But this issue is marked as "bug", there is no bug. You interpret the AnatomicalOrientation tag as something related to reference coordinate system, but it is not, this 3 letters code is orientation of matrix according to above definition.

#include <itkSpatialOrientation.h>

ImageType::Pointer image = reader->GetOutput();
itk::SpatialOrientationAdapter adapter;
const unsigned int orientation = static_cast<unsigned int>(
   adapter.FromDirectionCosines(image->GetDirection()));
switch (orientation)
{
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RIP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LIP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RSP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LSP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RIA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LIA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RSA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LSA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IRP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ILP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SRP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SLP:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IRA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ILA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SRA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SLA:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RPI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LPI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RAI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LAI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RPS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LPS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RAS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LAS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PRI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PLI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ARI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ALI:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PRS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PLS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ARS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ALS:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IPR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SPR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IAR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SAR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IPL:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SPL:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IAL:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SAL:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PIR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PSR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_AIR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ASR:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PIL:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PSL:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_AIL:
    break;
  case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ASL:
    break;
  default:
    break;
  }

issakomi commented 5 years ago

@aylward I have never seen definition for 3-letters orientation code in DICOM (not sure it does exist), coordinate system is defined here. Thanks heaven it is same with ITK's coordinate system. But terminology may be different, usually DICOM's is named DICOM LPS, same with ITK's RAI, same with Nrrd's left-posterior-superior. But not the same - Slicer's RAS, Slicer has different coordinate system.

Difference in terminology is usage of from or to notation.

aylward commented 5 years ago

I agree that the definition is poorly worded.

The direction cosine matrix exists in a space. That space is defined by the AnatomicOrientation. That is, when moving in the physical x-direction (or in i-index direction with an identity direction matrix), then you are going toward L when in an LPS anatomic space. Changing the direction matrix will never change the fact that you're in an LPS space, but it will change how moving along the i-index direction maps to a movement in that space. It tells you that the first i-index pixel you access in memory is to the right of the second pixel.

The code cited by @issakomi seems to only consider direction cosines, then it is perhaps describing how the ijk-index spans (i.e., is oriented within) a specific (e.g., ILS) space. This is different than telling us how that larger space is oriented (e.g., if it is ILS).

On Fri, Jun 14, 2019 at 3:18 PM issakomi notifications@github.com wrote:

@agirault https://github.com/agirault Thank you for responce. But this issue is marked as "bug", there is no bug. You interpret the AnatomicalOrientation tag as something related to reference coordinate system, but it is not, this 3 letters code is orientation of matrix according to above definition.

include

ImageType::Pointer image = reader->GetOutput(); itk::SpatialOrientationAdapter adapter; const unsigned int orientation = static_cast( adapter.FromDirectionCosines(image->GetDirection())); switch (orientation) { case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_AIL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ALS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ARI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ASR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IAR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ILA: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IPL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IRP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LAI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LIP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LPS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LSA: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PIR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PLI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PRS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PSL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RAS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RIA: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RPI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RSP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SAL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SLP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SPR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SRA: break; default: break; }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJL7WT36QCPHNSJ54QETP2PVH3A5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXXXGUI#issuecomment-502231889, or mute the thread https://github.com/notifications/unsubscribe-auth/AACEJLYSKE2DQIFX2GNK2HLP2PVH3ANCNFSM4HX5GQMA .

--

Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives, Kitware, Inc., North Carolina

Kitware: Advancing the frontiers of understanding by developing innovative open-source software platforms and integrating them into research, processes, and products.

aylward commented 5 years ago

Ah - I misunderstood what you meant by coordinate system - your mention of "from / to notation" cleared it up for me. I agree that it is very confusing - even with nearly infinite documentation, I think it borderlines on being a bug in Slicer, if what you say is true (and it seems that you've looked into it extensively already - thanks!)

MetaIO uses "to notation" and it uses this notation so that images could be moved between VTK and ITK; the MetaIO code base is shared between both of those libraries, but it looks like things got a bit mixed up along the way.

I thought DICOM also supported an LPS / RAI specification and patient orientation specification (e.g., I thought it used "HFS" in some tag to indicate a head first scan - slice ordering info), but I am probably wrong

I have happily avoided digging into DICOM for quite some time. Thanks!

On Fri, Jun 14, 2019 at 3:43 PM Stephen Aylward stephen.aylward@kitware.com wrote:

I agree that the definition is poorly worded.

The direction cosine matrix exists in a space. That space is defined by the AnatomicOrientation. That is, when moving in the physical x-direction (or in i-index direction with an identity direction matrix), then you are going toward L when in an LPS anatomic space. Changing the direction matrix will never change the fact that you're in an LPS space, but it will change how moving along the i-index direction maps to a movement in that space. It tells you that the first i-index pixel you access in memory is to the right of the second pixel.

The code cited by @issakomi seems to only consider direction cosines, then it is perhaps describing how the ijk-index spans (i.e., is oriented within) a specific (e.g., ILS) space. This is different than telling us how that larger space is oriented (e.g., if it is ILS).

On Fri, Jun 14, 2019 at 3:18 PM issakomi notifications@github.com wrote:

@agirault https://github.com/agirault Thank you for responce. But this issue is marked as "bug", there is no bug. You interpret the AnatomicalOrientation tag as something related to reference coordinate system, but it is not, this 3 letters code is orientation of matrix according to above definition.

include

ImageType::Pointer image = reader->GetOutput(); itk::SpatialOrientationAdapter adapter; const unsigned int orientation = static_cast( adapter.FromDirectionCosines(image->GetDirection())); switch (orientation) { case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_AIL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ALS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ARI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ASR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IAR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_ILA: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IPL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_IRP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LAI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LIP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LPS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_LSA: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PIR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PLI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PRS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_PSL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RAS: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RIA: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RPI: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_RSP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SAL: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SLP: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SPR: break; case itk::SpatialOrientation::ITK_COORDINATE_ORIENTATION_SRA: break; default: break; }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJL7WT36QCPHNSJ54QETP2PVH3A5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXXXGUI#issuecomment-502231889, or mute the thread https://github.com/notifications/unsubscribe-auth/AACEJLYSKE2DQIFX2GNK2HLP2PVH3ANCNFSM4HX5GQMA .

--

Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives, Kitware, Inc., North Carolina

Kitware: Advancing the frontiers of understanding by developing innovative open-source software platforms and integrating them into research, processes, and products.

--

Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives, Kitware, Inc., North Carolina

Kitware: Advancing the frontiers of understanding by developing innovative open-source software platforms and integrating them into research, processes, and products.

issakomi commented 5 years ago

@aylward

I think it borderlines on being a bug in Slicer, if what you say is true

Slicer team, AFAIK, will not accept this as bug. They say

However different medical applications use different definitions of this 3D basis.
Most common are the following bases:
LPS (Left, Posterior, Superior) is used in DICOM images and by the ITK toolkit
RAS (Right, Anterior, Superior) is similar to LPS with the first two axes flipped and used by 3D Slicer
Both bases are equally useful and logical.
It is just necessary to know to which basis an image is referenced.

aylward commented 5 years ago

Wow - I hadn't thought of that. You're right! I've seen that, but never fully realized the implications. Thanks!

On Fri, Jun 14, 2019 at 4:12 PM issakomi notifications@github.com wrote:

@aylward https://github.com/aylward

I think it borderlines on being a bug in Slicer, if what you say is true

Slicer team, AFAIK, will not accept this as bug. They say https://www.slicer.org/wiki/Coordinate_systems

However different medical applications use different definitions of this 3D basis. Most common are the following bases: LPS (Left, Posterior, Superior) is used in DICOM images and by the ITK toolkit RAS (Right, Anterior, Superior) is similar to LPS with the first two axes flipped and used by 3D Slicer Both bases are equally useful and logical. It is just necessary to know to which basis an image is referenced.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJL7JIQ5MA2HHZVH3WA3P2P3RPA5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXX3A3Q#issuecomment-502247534, or mute the thread https://github.com/notifications/unsubscribe-auth/AACEJL4XDCNEWCP5RTFJZILP2P3RPANCNFSM4HX5GQMA .

--

Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives, Kitware, Inc., North Carolina

Kitware: Advancing the frontiers of understanding by developing innovative open-source software platforms and integrating them into research, processes, and products.

agirault commented 5 years ago

There are two issues being discussed here:

lately, the difference between definitions of 3D basis. We seem to all agree that we need to account for:
- where to look at on the axis to describe it ("from RAI" = "to LPS", like for ITK)
- different coordinate systems (if we go with the "to" definition, Slicer is RAS, ITK is LPS)

For this issue, further discussions can be made to better describe this, as ITK/MetaIO is the only one using the "from" definition, and is prone to a lot of confusion as the last couple comments can confirm. And that's not new.

originally, the issue of the anatomical orientation tag being redundant with the transform matrix, which doesn't fit with the initial design. This was already described in comments above, summarized again by @aylward :

The direction cosine matrix exists in a space. That space is defined by the AnatomicalOrientation. That is, when moving in the physical x-direction (or in i-index direction with an identity direction matrix), then you are going toward L when in an LPS anatomic space. Changing the direction matrix will never change the fact that you're in an LPS space, but it will change how moving along the i-index direction maps to a movement in that space. It tells you that the first i-index pixel you access in memory is to the right of the second pixel.

We'll draft a well-documented post explaining the ins-and-outs of the issue, suggest a versioning update of the MetaIO format to restore this initial feature while maintaining backward-compatibility with how MetaIO has been used for the last 14 years, then share it in the ITK discourse forum to get feedback from the community.

aylward commented 5 years ago

Great summary and great plan!

On Mon, Jun 17, 2019 at 10:35 AM Alexis Girault notifications@github.com wrote:

There are two issues being discussed here:

lately, the difference between definitions of 3D basis. We seem to all agree that we need to account for:

where to look at on the axis to describe it ("from RAI" = "to LPS", like for ITK)

different coordinate systems (if we go with the "to" definition, Slicer is RAS, ITK is LPS)

For this issue, further discussions can be made to better describe this, as ITK/MetaIO is the only one using the "from" definition, and is prone to a lot of confusion as the last couple comments can confirm. And that's not new. https://itk.org/Wiki/Proposals:Orientation#Current_ITK_Usage_and_sources_of_confusion

originally, the issue of the anatomical orientation tag being redundant with the transform matrix, which doesn't fit with the initial design. This was already described in comments above, summarized again by @aylward https://github.com/aylward :

The direction cosine matrix exists in a space. That space is defined by the AnatomicalOrientation. That is, when moving in the physical x-direction (or in i-index direction with an identity direction matrix), then you are going toward L when in an LPS anatomic space. Changing the direction matrix will never change the fact that you're in an LPS space, but it will change how moving along the i-index direction maps to a movement in that space. It tells you that the first i-index pixel you access in memory is to the right of the second pixel.

We'll draft a well-documented post explaining the ins-and-outs of the issue, suggest a versioning update of the MetaIO format to restore this initial feature while maintaining backward-compatibility with how MetaIO has been used for the last 14 years, then share it in the ITK discourse forum to get feedback from the community.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJL4VJFQXMCEPBPWCAMLP26OJPA5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX3LXSI#issuecomment-502709193, or mute the thread https://github.com/notifications/unsubscribe-auth/AACEJL37L4YCQHZINYCILKTP26OJPANCNFSM4HX5GQMA .

--

Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives, Kitware, Inc., North Carolina

Kitware: Advancing the frontiers of understanding by developing innovative open-source software platforms and integrating them into research, processes, and products.

issakomi commented 5 years ago

in my apps i heavily use itk::SpatialOrientation::ITK_COORDINATEORIENTATION.... enum and compatibility of ITK and DICOM space, it is great advantage. I don't understand what is your goal? Rename RAI to LPS? There is no standard for that and i've never seen 48 orientation codes like in ITK, here and there is wording "DICOM LPS" (not defined in DICOM) and "Slicer RAS", nothing else. I also don't understand what is wrong with direction cosines in MetaImage and why should any additional transforms be applied? I will be not able to update to newer version of ITK, sorry, i have no time to re-write applications, i better stay with the old version of ITK and will back-port critical bugs... Also i have no time to participate in discussions, for me it is too clear. You are from Kitware, it is your playground, but i leave the discussion. Good luck.

aylward commented 5 years ago

I think an important message from @issakomi is to not break backward compatibility. I think that we should be able to maintain backward compatibility in this case (functionality and API), and it is something we should test heavily.

@issakomi - if I misunderstood your message, please let me know. I greatly appreciate the help that you have already given. Hopefully the end result will be to your liking (e.g., not require changes to your code) and perhaps make moving data between ITK and other applications (e.g., Slicer) more clear/defined/easy.

Thanks!

On Mon, Jun 17, 2019 at 11:54 AM issakomi notifications@github.com wrote:

i my apps i heavily use itk::SpatialOrientation::ITK_COORDINATEORIENTATION.... enum and full compatibility of ITK and DICOM space, it is great advantage. I don't understand what is your goal? Rename RAI to LSP? There is no standard for that and i've never seen 48 orientation codes like in ITK, here and where is wording "DICOM LPS" (not defined in DICOM) and "Slicer RAS", nothing else. I also don't understand what is wrong with direction cosines in MetaImage and why should any additional transforms be applied? I will be not able to updated to newer version of ITK, sorry, i have no time to re-write application, i better stay with old version of ITK and will backport critical bugs... Also i have no time to participate in discussions, for me it is too clear. You are from Kitware, it is you playground, but i leave the discussion. Good luck.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJLY5RHWNF5J43BFSWQTP26XTPA5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX3T67I#issuecomment-502742909, or mute the thread https://github.com/notifications/unsubscribe-auth/AACEJL24GQ3G7KMLA6KZEADP26XTPANCNFSM4HX5GQMA .

--

Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives, Kitware, Inc., North Carolina

Kitware: Advancing the frontiers of understanding by developing innovative open-source software platforms and integrating them into research, processes, and products.

lassoan commented 5 years ago

Very interesting to see this issue coming up again. I've raised this question 7 years ago and the conclusion at that time was that ITK's implementation was correct and if the documentation was not consistent then it was because it was not interpreted correctly (see details here).

Current AnatomicalOrientation tag is messed up because axis names are inverted (compared to DICOM, nrrd, ... and pretty much everything else) and the tag was present but has been ignored for really long time.

One safe option would be to adopt "space" tag from nrrd.

Another safe (and long-term time-saving) option would be to deprecate metaimage format. Nrrd file format was developed for exactly the same purpose as metaimage, but nrrd has advantages, such as having a clear definition of anatomical orientation/space tag (consistent with documentation), it has standard fields for describing axis kinds (very important for higher-dimensional images), and does not contain complicated and rarely-used features (such as multiple data files for a single header). If the decision is made to keep maintaining/improving metaimage format then it would be important to clean up and improve the specification (to be essentially even more similar to nrrd).

pieper commented 5 years ago

Good to see this all being carefully reviewed. I believe we all agree it would be a big problem if new code started interpreting old file header values differently (specifically, it would be a problem if ITK started treating AnatomicalOrientation as having geometric meaning when it was previously ignored).

I'll just add that in addition to carefully defining the layout of pixel samples with respect to patient space, we need to take care about the reference space for vector and tensor valued pixels. The nrrd concept of measurement frame nicely captures this and has been very useful in practice.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

lassoan commented 4 years ago

Please don't do this. I don't want to fight with robots.

Adding a "stale" label is fine. It is also OK to assign it to a "backlog". However, closing an issue would make it seem that the issue has been fixed or it was not valid in the first place, which is often not true.

I understand the desire to have less open issues, but automatically sweeping them under the rug will not improve quality and discourage community members to spend time with reporting issues.

The only valid use case I see for a stale-bot is to automatically close issues that are labelled as "more-information-needed". If more information is needed from the reporter but response does not arrive within reasonable time then it makes sense to close the issue (and doing it automatically saves some time for the maintainers).

agirault commented 4 years ago

@dzenanz @thewtex: I don't plan to work on this anytime soon sadly, but can you assign this issue to me so I don't lose track of it? Thank you.

dzenanz commented 4 years ago

Andras makes a valid point. Maybe we could change the bot's settings? @hjmjohnson @thewtex @blowekamp

thewtex commented 4 years ago

It appears that the stale-bot was configured not to close -- the comment and configuration are updated here: https://github.com/InsightSoftwareConsortium/ITK/pull/1495

lassoan commented 4 years ago

Perfect, thank you very much!

aylward commented 4 years ago

Back on topic....I've updated the MetaIO documentation, and hopefully it captures this discussion and provides a path forward...

See https://itk.org/Wiki/ITK/MetaIO/Documentation#Tags_Added_by_MetaImage

Also copied below...

AnatomicalOrientation

MET_STRING
This is a convenience tag, to be used and maintained by applications. Changing this tag in a MetaIO file or via the MetaIO API will not cause the MetaIO library to resample or change the direction matrix of an image. It is up to an application to correctly read, write, and maintain this tag.
The history of the interpretation of this tag is as follows:
- Labels: The labels used to define a space can be "from" labels or "to" labels, where "from" labels define the relative physical location of the origin of a space and "to" labels define the physical direction of movement of a space - thereby RAI "from" space is the same as LPS "to" space. DICOM, ITK, and nearly every application consider label space to be "to" labels.
- In an LPS "to" space ("to" space will be assumed in the remainder of this discussion), when moving in the x,y,z-directions of "space", then you are physically moving to the left.
- The direction matrix specifies how "index" i,j,k-directions (sometimes referred to as the in-memory directions of an image) map into x,y,z-directions. Changing the direction matrix will never change the fact that you're in an LPS space, but changing a direction matrix will change how moving along the i-index direction maps to a movement in the LPS space.
- Originally, AnatomicalOrientation was intended to define the space (LPS, RAS, etc) of an image; however, in early 2000s, its implementation was changed (and its documentation became unclear) to define how a patient was oriented in index space (it assumed an LPS space, and thus it became redundant with the direction matrix). These changes seem to have coincided with ITK's introduction of a direction matrix (thanks goes to Alexis Girault for hunting down this history!). For the past fifteen years++, ITK writes AnatomicOrientation based on the direction matrix and assumes an LPS space. See the code in itkSpatialOrientation.h that specifies a 3-letter code derived from an image's direction matrix and that 3-letter code is then used to specify the AnatomicalOrientation tag in MetaIO.
- Moving forward, when implementing new applications, it is recommended that developers use the definition currently implemented by ITK. It represents the most common use of MetaIO.

lassoan commented 4 years ago

Ok, this text clarifies that METAIO library will not reorient based on the AnatomicalOrientation tag. But should the application do it instead? The standard must clearly define this, since image geometry is such a fundamental information. If applications must ignore this field, too, then probably the field should be deprecated.

A new field with a different name could be introduced for anatomical orientation that is actually used (and follows usual naming conventions - LPS and not RAI).

If we intent to keep the metaimage format relevant, then new field should be introduced for defining dimensions (similarly to nrrd - space, time, color, list, etc).

aylward commented 4 years ago

I think we're in a good place already. This is a tag. It has a meaning. The meaning is defined now in this documentation and reflects how the code has been used in ITK for 15+ years. I don't think we need to change or add anything. We could add more tags to make it more general, but if we introduced a new tag for the few non-LPS images that are out there, then chances are that existing code won't be updated to use that new tag, and so we'll be introducing a way for existing code to fail.

Perhaps the documentation should more strongly state that this is how things are from now on - perhaps "moving forward" is too subtle?

On Mon, Dec 23, 2019 at 12:23 PM Andras Lasso notifications@github.com wrote:

Ok, this text clarifies that METAIO library will not reorient based on the AnatomicalOrientation tag. But should the application do it instead? The standard must clearly define this, since image geometry is such a fundamental information. If applications must ignore this field, too, then probably the field should be deprecated.

A new field with a different name could be introduced for anatomical orientation that is actually used (and follows usual naming conventions - LPS and not RAI).

If we intent to keep the metaimage format relevant, then new field should be introduced for defining dimensions (similarly to nrrd - space, time, color, list, etc).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJL6L2A4DA3PJHKQ3AMDQ2DX2XA5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHRR44Q#issuecomment-568532594, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACEJL2V5HXSUQW4H5SG7ALQ2DX2XANCNFSM4HX5GQMA .

-- Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives

aylward commented 4 years ago

Hi,

This is perhaps clearer regarding how things are defined now...

AnatomicalOrientation

MET_STRING
This is a convenience tag, to be used and maintained by applications. Changing this tag in a MetaIO file or via the MetaIO API will not cause the MetaIO library to resample or change the direction matrix of an image. It is up to an application to correctly read, write, and maintain this tag.
The history of the interpretation of this tag is as follows:
- Labels: The labels used to define a space can be "from" labels or "to" labels, where "from" labels define the relative physical location of the origin of a space and "to" labels define the physical direction of movement of a space - thereby RAI "from" space is the same as LPS "to" space. DICOM, ITK, and nearly every application consider label space to be "to" labels.
- In an LPS "to" space, when moving in the x,y,z-directions of "space", then you are physically moving to the left.
- The direction (cosines) matrix specifies how "index" i,j,k-directions (sometimes referred to as the in-memory directions of an image) map into x,y,z-directions. Changing the direction matrix will never change the fact that you're in an LPS space, but changing a direction matrix will change how moving along the i-index direction maps to a movement in that LPS space.
- Originally, AnatomicalOrientation was intended to define the space (LPS, RAS, etc) of an image; however, in early 2000s, its implementation was changed (and its documentation became unclear) to define how a patient was oriented in index space (it assumed an LPS space, and thus it became redundant with the direction matrix). These changes seem to have coincided with ITK's introduction of a direction matrix (thanks goes to Alexis Girault for hunting down this history!). For the past fifteen years++, ITK writes AnatomicOrientation based on the direction matrix and assumes an LPS "to" space. See the code in itkSpatialOrientation.h that specifies a 3-letter "from" label derived from an image's direction matrix and that 3-letter code is then used to specify the AnatomicalOrientation tag in MetaIO. Therefore, for example, in a CT scan the AnatomicOrientation is typically RAS or RAI (depending on the direction of the sequential slices).
Proper Usage
- When implementing new applications, developers must follow the convention maintained by ITK for the past 15+ years.
- MetaIO and ITK are defined in LPS "to" space - the same space used by DICOM images.
- AnatomicOrientation defines the "from" orientation of an image's index space, as computed in itkSpatialOrientationAdapter.h. That is, it is derived from the direction (cosines) matrix, under the assumption of an LPS "to" space. Since an axis might not be exactly R/L, A/P, I/S aligned, the labels provides by AnatomicOrientation are approximate, using the algorithm in itkSpatialOrientationAdapter.h::FromDirectionCosines().
  - This notation roughly mimics DICOM's convention for the "patient position" tag (https://dicom.innolitics.com/ciods/basic-voice-audio/general-series/00185100) whereby, for example, HFS refers to "head first-supine" (a "from" label system").

lassoan commented 4 years ago

This description is great. It gives a good explanation for all the issues related to this field.

I just brought up the other fields because if you want to invest time into this file format then it is important to add a few basic features. But it is perfectly fine to leave this file format as is, because nrrd can be used instead (similarly simple but defines data axes more clearly).

aylward commented 4 years ago

Agreed. If you need to handle the corner cases, NRRD is the way to go. I like keeping MetaIO simpler and more straightforward.

Thanks for the help with it. If you have ideas for clarifying the descriptions, please post them. Otherwise, I will close this issue in a few days.

Thanks, Stephen

On Mon, Dec 23, 2019 at 1:35 PM Andras Lasso notifications@github.com wrote:

This description is great. It gives a good explanation for all the issues related to this field.

I just brought up the other fields because if you want to invest time into this file format then it is important to add a few basic features. But it is perfectly fine to leave this file format as is, because nrrd can be used instead (similarly simple but defines data axes more clearly).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/issues/1017?email_source=notifications&email_token=AACEJLZU43TIFX5HTRYSZNDQ2EAFHA5CNFSM4HX5GQMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHRV2BY#issuecomment-568548615, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACEJL7QT57K5UAPGWIS4PDQ2EAFHANCNFSM4HX5GQMA .

-- Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

dzenanz commented 3 months ago

It would be good if you wrote which commit or PR accomplished it.

aylward commented 3 months ago

Closed by notes given in final three comments and changes made to MetaIO documentation wiki.