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

Some DICOM volumes are loaded flipped along Z axis in ITK-v5.1rc01 #1587

Open
lassoan opened this issue Feb 2, 2020 · 9 comments
Open

Some DICOM volumes are loaded flipped along Z axis in ITK-v5.1rc01 #1587

lassoan opened this issue Feb 2, 2020 · 9 comments
Labels
Milestone

Comments

@lassoan
Copy link

@lassoan lassoan commented Feb 2, 2020

Description

Some volumes are flipped along their Z axis (ordering of slices is inverted). Other axes are not inverted, so it is not a rotation but a mirroring along one axis. It worked well in the previous ITK 5 release.

image

Steps to Reproduce

Load the DICOM volume using ITK-5.0 and ITK-5.1rc01 and compare them in a sagittal view.

Expected behavior

They should look the same.

Actual behavior

There is a flip along the IS axis.

Reproducibility

I can provide an anonymized data set on request but it is not for public sharing.

Versions

ITK-v5.1rc01

Environment

Windows, Visual Studio 2015

@lassoan lassoan added the type:Bug label Feb 2, 2020
@pieper

This comment has been minimized.

Copy link
Contributor

@pieper pieper commented Feb 2, 2020

I can reproduce with public data (this was series 7 from patient 0000 in this TCIA collection.

Also can confirm that this appears on a Mac build. I'll try to come up with a simple way to replicate the issue.

@thewtex thewtex added this to the ITK v5.1.0 milestone Feb 5, 2020
@jhlegarreta

This comment has been minimized.

Copy link
Member

@jhlegarreta jhlegarreta commented Feb 5, 2020

May be related to #1163 ?

@dzenanz

This comment has been minimized.

Copy link
Member

@dzenanz dzenanz commented Feb 5, 2020

No, #1163 is about different naming conventions for orientations, not orientations being different.

@lassoan

This comment has been minimized.

Copy link
Author

@lassoan lassoan commented Feb 5, 2020

We have done some more testing and found that file names matter: by renaming files from 0001, 0002, 003, ... to 0400, 0399, 0398, ... we could make the image flip upside down.

@pieper did some more testing and it seems that the issue is related to how the DICOM reader is used in Slicer (we collect the DICOM files and pass the filename list to ITK). Something has changed in ITK that changed the behavior, for example, the filename list was previously sorted internally in ITK, but now it is considered to be already sorted?

@dzenanz

This comment has been minimized.

Copy link
Member

@dzenanz dzenanz commented Feb 5, 2020

@malaterre and @issakomi might want to pitch in.

@pieper

This comment has been minimized.

Copy link
Contributor

@pieper pieper commented Feb 5, 2020

Some more detail on what @lassoan reported, when users load data through Slicer's DICOM module, we sort the data according to geometry (and do some other consistency checks) and send the sorted data to itk for reading.

When users drop an arbitrary file on the application, we delegate down through some very old logic that was created in the early days of ITK and Slicer which tries to guess what the user might want to do with the data. It is in this old code or something related that things no longer work. Most of the original authors are long gone, and this old spaghetti code should go too, but it's odd that it worked for the last 14 years and now something broke.

@issakomi

This comment has been minimized.

Copy link
Contributor

@issakomi issakomi commented Feb 5, 2020

We have done some more testing and found that file names matter: by renaming files from 0001, 0002, 003, ... to 0400, 0399, 0398, ... we could make the image flip upside down.

May be the change, s. link
https://sourceforge.net/p/gdcm/gdcm/ci/2b481c069485d364c1160ef6eb14fa9e52e48879/

I only guess, sorry, i don't use this code in my app and didn't try to reproduce the issue.

@issakomi

This comment has been minimized.

Copy link
Contributor

@issakomi issakomi commented Feb 5, 2020

@lassoan
to test, may be, s.

+/*
  else if ( ImageNumberOrdering(fileSet ) )
  {
  return ;
  }
+*/


@malaterre

This comment has been minimized.

Copy link
Member

@malaterre malaterre commented Feb 6, 2020

@lassoan
to test, may be, s.

+/*
  else if ( ImageNumberOrdering(fileSet ) )
  {
  return ;
  }
+*/

This could be the issue here. But I was asked recently to resurect this logic to make an ITK test pass, so I would not know which way to go then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.