WIP: ENH: Increase itk::DCMTKFileReader class coverage

[jhlegarreta]

Add a new test to the IODCMTK module to increase the coverage of the
itk::DCMTKFileReader::GetElement* methods.

PR Checklist

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)
• Added test (or behavior not changed)
• Added license to new files (if any)

[jhlegarreta]

A few notes:

• I know the module gets less use than GDCM, but if it is in the toolkit, it should get some more testing.
• I could not compile the DCTMK test module:

Build started...
1>------ Build started: Project: ITKIODCMTKTestDriver, Configuration: Debug x64 ------
1>itkDCMTKGetDicomTagsTest.cxx
1>C:\SDKs\ITK\itk-head\ITK\Modules\IO\DCMTK\include\itkDCMTKFileReader.h(27,10): fatal error C1083:
Cannot open include file: 'dcmtk/dcmdata/dcdict.h': No such file or directory
1>Done building project "ITKIODCMTKTestDriver.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

Using the built-in DCMTK (i.e. not a system DCMTK) I cannot see where the error stems from. I do see the file on my build tree in \itk-head-Bin\Modules\ThirdParty\DCMTK\ITKDCMTK_ExtProject\dcmdata\include\dcmtk\dcmdata\dcdict.h

I submitted an experimental build to see if that helps other folks to try to reproduce the issue:
https://open.cdash.org/build/7503588

Thus, I could not compile and run the test to see its errors.

Not sure if the CIs have the module flag set to On so they may come up green.

• Not sure if the test name is the most appropriate one. Suggestions are welcome.
• I did not found non-empty tags in the used DICOM data to exercise other methods like, GetElementCSorOB, etc. Adding another test that uses another set of DICOM data could allow querying such elements/exercising those methods. Best would be to make the test quantitative for that.
• @thewtex In order to make the test quantitative, I'm wondering whether we have CMake-enabled ways to compare a potential txt output file with the read tags to a baseline with the expected tags. If not, do you have some suggestion?

[dzenanz]

Making the test quantitative would be good, but that can be a follow-up commit. This is good as-is.

[jhlegarreta]

Making the test quantitative would be good, but that can be a follow-up commit. This is good as-is.

Sounds reasonable. Note that I have not been able to compile the module, and hence nor I have tested it, and I would not merge this until it gets tested.

The CI builds did not set the module to ON, and thus, even if they have come green, it might well be the case that the test does not compile/pass:

2021-10-10T01:29:38.0772539Z 595: //Request building ITKIODCMTK (non-remote)
2021-10-10T01:29:38.0773102Z 595: Module_ITKIODCMTK:BOOL=OFF

[dzenanz]

I will test it locally and report back.

[dzenanz]

It fails to compile for me too:

7>itkDCMTKGetDicomTagsTest.cxx
7>C:\Dev\ITK-git\Modules\IO\DCMTK\include\itkDCMTKFileReader.h(27,10): fatal error C1083: Cannot open include file: 'dcmtk/dcmdata/dcdict.h': No such file or directory

[dzenanz]

The offending file is located in C:\Dev\ITK-2019\Modules\ThirdParty\DCMTK\ITKDCMTK_ExtProject\dcmdata\include\dcmtk\dcmdata\dcdict.h, and the only mention of DCMTK in additional include directories is C:\Dev\ITK-git\Modules\IO\DCMTK\include.

[jhlegarreta]

and the only mention of DCMTK in additional include directories is C:\Dev\ITK-git\Modules\IO\DCMTK\include

Meaning that including the directory is missing in the CMakeLists.txt file?

I did try to add it manually to my MSVC properties, but it did not solve the problem. But maybe I did not do it appropriately.

Also, Brad's AWS build tests the module, so it would be weird that this should not happen on Linux:
https://open.cdash.org/build/7505424/notes

[dzenanz]

ITKDCMTK_INCLUDE_DIRS seems to have appropriate dirs, being C:/Dev/ITK-2019/Modules/ThirdParty/DCMTK/ITKDCMTK_ExtProject/dcmdata/include;.... Manually adding the first entry to Visual Studio project ITKIODCMTKTestDriver, changes the error to:

5>------ Build started: Project: ITKIODCMTKTestDriver, Configuration: RelWithDebInfo x64 ------
5>itkDCMTKGetDicomTagsTest.cxx
5>C:\Dev\ITK-2019\Modules\ThirdParty\DCMTK\ITKDCMTK_ExtProject\dcmdata\include\dcmtk/dcmdata/dcdict.h(26,10): fatal error C1083: Cannot open include file: 'dcmtk/config/osconfig.h': No such file or directory
5>itkDCMTKImageIOTest.cxx
5>itkDCMTKImageIONoPreambleTest.cxx
5>Generating Code...
5>Done building project "ITKIODCMTKTestDriver.vcxproj" -- FAILED.

Adding the entire content of ITKDCMTK_INCLUDE_DIRS to VS project gets it "building":

2>------ Build started: Project: ITKIODCMTKTestDriver, Configuration: RelWithDebInfo x64 ------
2>ITKIODCMTKTestDriver.cxx
2>itkDCMTKGetDicomTagsTest.cxx
2>C:\Dev\ITK-2019\Modules\ThirdParty\DCMTK\ITKDCMTK_ExtProject-build\config\include\dcmtk/config/arith.h(1,1): warning C4335: Mac file format detected: please convert the source file to either DOS or UNIX format
2>C:\Dev\ITK-git\Modules\IO\DCMTK\include\itkDCMTKFileReader.h(307,1): error C2440: 'static_cast': cannot convert from 'T' to 'TType'
2>        with
2>        [
2>            T=Float64
2>        ]
2>        and
2>        [
2>            TType=std::string
2>        ]
2>C:\Dev\ITK-git\Modules\IO\DCMTK\include\itkDCMTKFileReader.h(307,1): message : No constructor could take the source type, or constructor overload resolution was ambiguous
2>C:\Dev\ITK-git\Modules\IO\DCMTK\test\itkDCMTKGetDicomTagsTest.cxx(92): message : see reference to function template instantiation 'int itk::DCMTKFileReader::GetElementDS<std::string>(const unsigned short,const unsigned short,unsigned short,TType *,const bool) const' being compiled
2>        with
2>        [
2>            TType=std::string
2>        ]
2>itkDCMTKImageIOTest.cxx
2>itkDCMTKRGBImageIOTest.cxx
2>itkDCMTKSeriesReadImageWrite.cxx
2>itkDCMTKSeriesStreamReadImageWrite.cxx
2>itkDCMTKImageIOOrthoDirTest.cxx
2>itkDCMTKMultiFrame4DTest.cxx
2>itkDCMTKLoggerTest.cxx
2>itkDCMTKImageIOSlopeInterceptTest.cxx
2>itkDCMTKImageIOMultiFrameImageTest.cxx
2>itkDCMTKImageIONoPreambleTest.cxx
2>Generating Code...
2>Done building project "ITKIODCMTKTestDriver.vcxproj" -- FAILED.

I don't know where the fix needs to be applied, but it is almost certainly somewhere in ITK's DCMTK-related CMake code.

[stale]

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

[jhlegarreta]

This is still relevant stale bot. I am willing to take up this as soon as #2817 gets resolved.

[issakomi]

Strange, my viewer and also dcmdump show
(0019,105a) FL 414273984 # 4, 1 AcquisitionDuration
not 1304791694. Somehow strange values for float, there may be an issue in the file.

[jhlegarreta]

Strange, my viewer and also dcmdump show
(0019,105a) FL 414273984 # 4, 1 AcquisitionDuration
not 1304791694. Somehow strange values for float, there may be an issue in the file.

Not sure where I got the value from :-S. I probably used an online viewer, which might not be fully trustworthy.

I am not able to build the module locally, so I was relying on an online tool rather than installing locally some library/tool. But I am willing to fix it if anybody can point me to a dependable tool that I could use to potentially get the accurate values for more tags.

[jhlegarreta]

Also applicable to all private tags in the test, it is correct to access private tags (odd group) using private creator, not just by group and element.

Can you elaborate on this? If there is a better way to access the tags, I may have a look to also add them (?).

[issakomi]

This particular tag is also known for DCMTK (is in private dictionary), so it is OK.

[issakomi]

I am not able to build the module locally, so I was relying on an online tool rather than installing locally some library/tool.

I have no problem with Linux build and internal DCMTK. I remember with external and at least static libraries there were problems too, something with missing libraries (they were there, but not found).

P.S.
I have to admit I am not experienced with DCMTK, actually I am testing it for PACS stuff, it seems to be clean and well maintained library.

[jhlegarreta]

Making the test quantitative would be good, but that can be a follow-up commit. This is good as-is.

I may give a go at this. If the module is turned on by the CIs, they'll tell about the tag contents. I'd think of two ways to make this quantitative:

• Write the expected tag values to a text file, provide it as the input to the test, read each line and compare each value with the value read from the DICOM files.
• Write the values read from the DICOM files to a text file, provide a text file with the expected tag values as a baseline to CMake, and let it do the comparison job.

If the latter is possible, it involves less code, and allows for improvements (e.g. reading/comparing more tags) without minimal changes to the test code.

@dzenanz Wondering whether the CMake compare is able to compare two text files (i.e. without using the content links).

[issakomi]

DCMTK has own tests, BTW, there are some macros too e.g. OFCHECK_EQUAL
https://github.com/DCMTK/dcmtk/tree/master/dcmdata/tests

[jhlegarreta]

DCMTK has own tests

👍 Here we are trying to make sure that ITK's DCMTK IO object reads what it is supposed to read.

BTW, there are some macros too e.g. OFCHECK_EQUAL
https://github.com/DCMTK/dcmtk/tree/master/dcmdata/tests

👍 Thanks Mihail.

[dzenanz]

CMake can compare files (syntax), but only binary comparison. It should work in this case, and it is less work to set up than read the file and compare ourselves. The downside is that error message will be "file are different", rather than more meaningful messages we can craft in the custom code. If you have time, you can go the custom comparison route. Otherwise let CMake compare the outputs.

P.S. I don't know how easy or hard it is to turn CMake -E compare_files into a test.

[dzenanz]

Slicer should be able to read DICOM tags.

[jhlegarreta]

P.S. I don't know how easy or hard it is to turn CMake -E compare_files into a test.

Yes, that one I had found yesterday, but did not find out whether our compare argument does also accept text files.

[issakomi]

FYI, some parts are too bad for testing and coverage, e.g. this function.
It takes Bits Allocated (0x0028, 0x0100) with wrong name SamplesPerPixel and assumes that if Bits Allocated is 8 or 16 it means that the image is scalar, otherwise RGB or VECTOR, it is wrong. RGB image has of course Bits Allocated 8 or 16 too and Samples per Pixel (0x0028,0x0002) 3. And e.g. 32 bits dose (RT) image is not a vector. Also Photometric Interpretation should be checked. There were some very very old ACR-NEMA images with bitsallocated 24 for RGB, extremely rare.

IOPixelEnum
DCMTKFileReader::GetImagePixelType() const
{
  unsigned short SamplesPerPixel;
  if (this->GetElementUS(0x0028, 0x0100, SamplesPerPixel, false) != EXIT_SUCCESS)
  {
    return IOPixelEnum::UNKNOWNPIXELTYPE;
  }
  IOPixelEnum pixelType;
  switch (SamplesPerPixel)
  {
    case 8:
    case 16:
      pixelType = IOPixelEnum::SCALAR;
      break;
    case 24:
      pixelType = IOPixelEnum::RGB;
      break;
    default:
      pixelType = IOPixelEnum::VECTOR;
  }
  return pixelType;
}

Edit:
this function is unused, pixel type is processed in ReadImageInformation (also with issues, but much better)

[jhlegarreta]

@issakomi Thanks for the investigation. I'd be more than happy to add more test cases to test the relevant methods if the appropriate DICOM data can be shared.

[issakomi]

@issakomi Thanks for the investigation. I'd be more than happy to add more test cases to test the relevant methods if the appropriate DICOM data can be shared.

I think i have mentioned a bug, better say forgotten garbage in DCMTKFileReader class.
There are many test files for GDCM tests. But i think they should be better tested manually first to get some knowledge of the class, e.g. we don't know does PALETTE_COLOR works or not.

The test is here, it is complete minimal standalone test. Not intended to be included in test suite.

#include "itkImage.h"
#include "itkImageFileReader.h"
#include "itkDCMTKImageIO.h"
#include "itkDCMTKFileReader.h"
#include <iostream>

int main(int, char **)
{
  auto imageIO = itk::DCMTKImageIO::New();
  imageIO->SetFileName("../data/raw-RGB.dcm");
  try
  {
    imageIO->ReadImageInformation();
  }
  catch (const itk::ExceptionObject & ex)
  {
    std::cout << ex.GetDescription() << std::endl;
  }
  itk::IOPixelEnum pixel_type1 = imageIO->GetPixelType();
  std::cout << "DCMTKImageIO reported Pixel Type  "
    << imageIO->GetPixelTypeAsString(pixel_type1)
    << std::endl;

  // GetImagePixelType() is unused garbage
  itk::DCMTKFileReader dcmtkFileReader;
  dcmtkFileReader.SetFileName("../data/raw-RGB.dcm");
  dcmtkFileReader.LoadFile();
  itk::IOPixelEnum pixel_type2 = dcmtkFileReader.GetImagePixelType();
  std::cout << "DCMTKFileReader reported Pixel Type  "
    << imageIO->GetPixelTypeAsString(pixel_type2)
    << std::endl;
  return 0;
}

DCMTKImageIO reported Pixel Type  rgb
DCMTKFileReader reported Pixel Type  scalar

Sorry, but i am out and will not do any more work on DCMTK in the near future.

[issakomi]

GetImagePixelType() is obviously too bad and should be fixed or even better removed from the class.

[jhlegarreta]

The ideal thing would be to fix it. If itk::DCMTKImageIO (maybe through itk::ImageIOBase) or the itk::DCMTKImageIO::ReadImageInformation are doing a better job in this, then part of the code might be valid for here (haven't looked). If we do not find a way to fix it, then it should be deprecated (and yet be tested so that at least its behavior is consistent) prior to being removed.

[issakomi]

itk::DCMTKImageIO::ReadImageInformation is in fact OK, not ideal too, i don't like that SamplesPerPixel=4 is assumed to be RGBA, in fact it could be e.g. CMYK. But is really not important, currently there are no DICOM files with SamplesPerPixel=4 (CMYK, ARGB), they were deprecated long long ago. So in 99.999% ReadImageInformation works fine. There are more to test (manually first), e.g. how DCMKT handles YBR images, PALETTE_COLOR. Maybe later.

[issakomi]

and yet be tested so that at least its behavior is consistent) prior to being removed

test are always good, but not including known completely broken stuff into test suite, sorry. The test text files can not match too, sorry.

[jhlegarreta]

Thanks for all this insight. Will see when I get the time to push this, as I am still being blocked by #2817 to be able to properly do this.

[jhlegarreta]

Now blocked by #3820.

[hjmjohnson]

Closing as superseded by #6189, which carries this work forward to a buildable, runnable state on current upstream/main.

The new PR addresses everything in this thread:

• The 3.5-year-old blocker (Including itkDCMTKFileReader.h in a test file produces a compiler error #3820) is fixed in commit 1 of ENH: Add itkDCMTKGetDicomTagsTest, fix #3820 (supersedes #2796) #6189: a narrowly-scoped target_include_directories() on ITKIODCMTKTestDriver that pulls in the DCMTK ExternalProject's include dirs without changing the IODCMTK module's public DEPENDS surface. Tests can now #include "itkDCMTKFileReader.h" directly.
• All of @issakomi's specific concerns are addressed:
  • The unsigned short = "????" compile error → rewritten
  • The 1304791694 wrong-by-transcription value → corrected to actual 414273984.0 (verified against the on-disk fixture with pydicom before authoring)
  • PhotometricInterpretation now read as CS string "MONOCHROME2" instead of unsigned short
  • The buggy GetImagePixelType() test was dropped per @malaterre's earlier feedback that the method itself is questionable
• Structural problems (3 duplicate itk_add_test names, comma-list-to-argv[1] mismatch, wholesale gersemi reformat) → all resolved
• Test now passes locally on macOS in 3.59s — pre-commit run --all-files exits 0

The two test invocations targeting RGBDicomSeries/raw-RGB.dcm and RTDoseDicomSeries/32bitDoseRTImage.dcm are dropped because those fixtures were never published to ITKTestingData. The Input/DicomSeries/Image0075.dcm invocation is preserved and exercises 13 distinct GetElement* accessors.

@jhlegarreta — your authorship of the original test concept is preserved via Co-Authored-By: trailers on the new commits. Thanks for opening this in 2021 even when the build couldn't yet support it. @issakomi — your review feedback shaped the corrected test; thank you.

Closing this PR; please follow the discussion on #6189.