GDCM Secondary Capture spatial metadata

[thewtex]

Support reading Secondary Capture spatial metadata with GDCM. Addresses issue #4109 , reading Visible Male RGB secondary capture spatial metadata. Also targeting support for highdicom generated SC re: https://github.com/ImagingDataCommons/highdicom/issues/247.

This is a follow-up to #4111.

The corresponding patch was submitted to GDCM in https://github.com/malaterre/GDCM/pull/158 and https://github.com/malaterre/GDCM/pull/167.

Add tests with the Visible Male data.

[thewtex]

Support has been merged into GDCM, thanks @malaterre @issakomi

[fedorov]

@thewtex thank you for working on this!

I will let @pieper to re-review and approve this.

[fedorov]

@thewtex I believe we discussed it at the last meeting that ITK used in Slicer (and in dcmqi) will be updated after ITK upstream update. Is this the plan, or should that be handled separately?

[thewtex]

@thewtex I believe we discussed it at the last meeting that ITK used in Slicer (and in dcmqi) will be updated after ITK upstream update. Is this the plan, or should that be handled separately?

I think @jcfr knows the ITK/Slicer update plans.

This patch will be included in ITK 5.4rc3, tentatively this week.

[thewtex]

Merging for the 5.4rc3 release.

[seanm]

@thewtex I've just bisected f545dd8f5b0ea5fb399da3586aef841d7a183e7c as the cause of a test failure in our application. We have some tests that write out DICOM files and compare the result to known-good DICOM files, and the comparison now fails. I haven't yet investigated deeper than doing the bisect. But just FYI...

[jcfr]

I think @jcfr knows the ITK/Slicer update plans.

Ditto :rocket: I will prepare the upgrade (ITK, SimpleITK and DCMTK) but will wait to hear back from @seanm before moving forward with the integration.

[malaterre]

@thewtex I've just bisected f545dd8 as the cause of a test failure in our application. We have some tests that write out DICOM files and compare the result to known-good DICOM files, and the comparison now fails. I haven't yet investigated deeper than doing the bisect. But just FYI...

Could you include more context. What is the test supposed to validate? Make sure to read DICOM CP 2330 and cc me if regression remains. thanks

[thewtex]

@seanm as @malaterre mentioned, there may be differences in output spatial metadata per DICOM CP 2330, but these are expected.

[seanm]

So my coworker has reduced it to a small test case. Attached here: repro-case.zip

For some DICOM files at least, itk::GDCMImageIO is giving different results for spacing before and after this change.

With 193a2ed744 (aka f545dd8~1) the test app outputs:

Origin = [0, 0, 0]
Spacing = [0.5, 0.5, 1]
Size = [3, 4, 3]

With f545dd8 it outputs:

Origin = [0, 0, 0]
Spacing = [1, 1, 1]
Size = [3, 4, 3]

[issakomi]

So my coworker has reduced it to a small test case. Attached here: repro-case.zip

For some DICOM files at least, itk::GDCMImageIO is giving different results for spacing before and after this change.

With 193a2ed (aka f545dd8~1) the test app outputs:

Origin = [0, 0, 0]
Spacing = [0.5, 0.5, 1]
Size = [3, 4, 3]

With f545dd8 it outputs:

Origin = [0, 0, 0]
Spacing = [1, 1, 1]
Size = [3, 4, 3]

S. here:

  case MediaStorage::SecondaryCaptureImageStorage:
    if( ImageHelper::SecondaryCaptureImagePlaneModule ) {
      // Make SecondaryCaptureImagePlaneModule act as ForcePixelSpacing
      // This is different from Basic Pixel Spacing Calibration Macro Attributes
      t = Tag(0x0028,0x0030);
    } else {
      t = Tag(0x0018,0x2010);
    }
    break;

This change was not in the first PR https://github.com/malaterre/GDCM/pull/158 (and not in the PR https://github.com/malaterre/GDCM/pull/167/) and the issue wouldn't happen with the PRs, it appears in the very final version.

[thewtex]

@seanm thanks for checking and providing the reproducible example data.

@issakomi thanks for the investigation.

@dclunie can you confirm that this is the expected output?

[seanm]

The DICOM files I provided were created by our app (via ITK, via GDCM), and the intended spacing when they were created was 0.5. So unless there is/was also a bug when writing files, the newer reader behaviour seems wrong to me.

[dclunie]

@issakomi given that your test set contains Nominal Scanned Pixel Spacing (0018,2010) only (with a 0.5mm value) and no other spacing related attributes, I would say the correct behavior for that test set is to return 0.5.

I assume that a return value of 1 is the "default" (which I am not excited about since there is no reason it should be assumed to be 1 or anything else ... I might have used 0 to force a failure if it is used).

Same goes for the third spacing value - there is no reason it should be 1 (and not, for example, 0) since there is no between slices spacing information in your test files.

This has nothing to do with the case when Image Position (Patient), Image Orientation (Patient) and Pixel Spacing (0028,0030) are present, which would then override any Nominal Scanned Pixel Spacing (0018,2010) present (which ideally it would not be in that situation).

FYI, the optional presence of the ImagePlane Module and accompanying note about relationship of spacing attributes is now standardized.

[issakomi]

@issakomi given that your test set contains Nominal Scanned Pixel Spacing (0018,2010) only (with a 0.5mm value) and no other spacing related attributes, I would say the correct behavior for that test set is to return 0.5.

Thank you. Yes, in fact the dataset only has a Nominal Scanned Pixel Spacing (0018,2010) and this attribute is ignored. This is erroneous.

This has nothing to do with the case when Image Position (Patient), Image Orientation (Patient) and Pixel Spacing (0028,0030) are present, which would then override any Nominal Scanned Pixel Spacing (0018,2010) present (which ideally it would not be in that situation).

This was proposed by https://github.com/malaterre/GDCM/pull/158 and https://github.com/malaterre/GDCM/pull/167. I am not sure why the final version is different.

[thewtex]

A follow-up can be found:

• https://github.com/InsightSoftwareConsortium/ITK/pull/4574
• https://github.com/malaterre/GDCM/pull/170

This was proposed by malaterre/GDCM#158 and malaterre/GDCM#167. I am not sure why the final version is different.

I am not sure, but there could be a reasonable attempt to make the function ImageHelper::GetSpacingTagFromMediaStorage do the work of returning the spacing tag for secondary capture, according to its name. Since the logic required makes it insufficient to return a single tag, I added a warning to flag that this function should not be called with Secondary Capture images. This could be elevated to throwing an exception if desired.

Thank you, @seanm, for finding and reporting the regression and providing the reproducible example. This has been added to the test suite.

Thank you, @issakomi , @malaterre , @pieper , for working on the patches to improve DICOM support.

Thank you, @dclunie, @fedorov for working on clarification and improvements to the standard. This has been a long-time thorn and impediment for DICOM adoption. I am glad we are making progress.

Please review the patch by Monday: the community is understandably anxious to see the ITK 5.4 release minted, and we want to get it out.

[seanm]

Thank you, @seanm, for finding and reporting the regression and providing the reproducible example. This has been added to the test suite.

We've tried git master and we're back to the same behaviour as ITK 5.3. Thanks for fixing that!

However like @dclunie said:

there is no between slices spacing information in your test files

Indeed our files have neither 0018|0050 nor 0018|0088. We created those DICOM files via ITK/GDCM. What did we fail to do to have them?

Same goes for the third spacing value - there is no reason it should be 1 (and not, for example, 0)

We agree 0.0 would be a better fallback, because 1.0 mm is a very common scan thickness, and one might think it's correct.

[thewtex]

@seanm thanks for verifying!

The tag used for that dataset is Nominal Scanned Pixel Spacing (0018,2010).

Setting the spacing to 0.0 would not be correct. Assigning an invalid default value to the spacing would be a hack approach to indicate that the spacing is not explicitly defined in the DICOM metadata. If the desire is to check whether spacing is explicitly defined, then it should be done at the application level, or an ivar on the DICOM ImageIO class to return a boolean that indicates as much. Or, use a different format that consistently and explicitly encodes spacing. A spacing of 1.0 is a reasonable default, will not result in Nan's and Inf's in analysis, and is consistent with the behavior of other ITK image IO interfaces.