Skip to content

Bugfixes in DCMTK#1179

Merged
dzenanz merged 3 commits intoInsightSoftwareConsortium:masterfrom
maekclena:bug_DCMTK
Aug 29, 2019
Merged

Bugfixes in DCMTK#1179
dzenanz merged 3 commits intoInsightSoftwareConsortium:masterfrom
maekclena:bug_DCMTK

Conversation

@maekclena
Copy link
Contributor

The first commit fixes #1125 and removes an outdated comment.
The second commit adds a metadata read for spacing (according to already existing comments in the code) to fix failing test itkDCMTKImageIOMultiFrameImageTest.

PR Checklist

@dzenanz
Copy link
Member

dzenanz commented Aug 20, 2019

I remember some discussions about this, e.g. #112 and #315. Were you aware of it @maekclena?

@maekclena
Copy link
Contributor Author

No, I did not know. It seems like the fallback behavior was mistakenly removed in #243.

@hjmjohnson
Copy link
Member

No, I did not know. It seems like the fallback behavior was mistakenly removed in #243.

I don't think it was mistakenly removed. My recollection is that it was purposefully removed because using SliceThinkness was determined to be the wrong approach. If 0018, 0088 is not present, then the correct behavior is to explicitly compute the distance between slices as described in

#112 (review)
and
#112 (comment)

@malaterre is an expert in DICOM conformance.

Copy link
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

Earlier discussions state that using 0018,0050 is a dangerous and incorrect way of determining z-spacing. This code was purposefully removed previously.

@dzenanz
Copy link
Member

dzenanz commented Aug 21, 2019

Please split this into two PRs. First commit is non-contentious, and can be merged immediately. That will allow reviews for the second commit independently.

@malaterre
Copy link
Member

No, I did not know. It seems like the fallback behavior was mistakenly removed in #243.

I don't think it was mistakenly removed. My recollection is that it was purposefully removed because using SliceThinkness was determined to be the wrong approach. If 0018, 0088 is not present, then the correct behavior is to explicitly compute the distance between slices as described in

#112 (review)
and
#112 (comment)

@malaterre is an expert in DICOM conformance.

  1. 0018,0088 is Spacing Between Slices and should represent the actual Z-spacing when present,
  2. Never (ever!) confuse Spacing Between Slices and Slice Thickness those are totally independent attributes
  3. During the course of ITK dev, it was found that Spacing Between Slices may be missing (eg. CT Image Storage), or incorrect. Computing the actual Z-spacing from the IOP & IPP make sure to report any missing slices or inconsistency. Which is the reason why it is acceptable to loop over all frames to (re)compute the Slice Interval.

Ref:

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.

The contentious changes have been removed. And disabling the failing test is reasonable.

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.

I think we should squash when merging.

@dzenanz dzenanz requested a review from blowekamp August 28, 2019 14:06
@dzenanz dzenanz merged commit c511095 into InsightSoftwareConsortium:master Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

itkDCMTKImageIO.h RGB Converter

5 participants