ENH: Add itkDCMTKGetDicomTagsTest, fix #3820 (supersedes #2796)#6189
Merged
hjmjohnson merged 2 commits intoInsightSoftwareConsortium:mainfrom May 3, 2026
Merged
Conversation
github-actions
Bot
added
type:Infrastructure
4 tasks
Member
|
Thanks for looking into this! I was just talking about features of GDCM and DCMTK in ITK with @zivy. |
Member
Author
|
@greptileai review this draft before I make it official |
This comment was marked as resolved.
This comment was marked as resolved.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…wareConsortium#3820) The ITK module ITKIODCMTK declares ITKDCMTK as PRIVATE_DEPENDS so the DCMTK headers do not leak through the toolkit's public include surface. But the public header itkDCMTKFileReader.h transitively #includes <dcmtk/dcmdata/dcdict.h>, so any test file that includes itkDCMTKFileReader.h fails to compile with: fatal error: 'dcmtk/dcmdata/dcdict.h' file not found This blocked PR InsightSoftwareConsortium#2796 from landing for over three years and is tracked as issue InsightSoftwareConsortium#3820. Add a narrowly-scoped target_include_directories() block in Modules/IO/DCMTK/test/CMakeLists.txt that adds the DCMTK ExternalProject's per-library include dirs and the generated configuration header dir to ITKIODCMTKTestDriver only. No change to the public DEPENDS surface of the IODCMTK module --- only the test driver target acquires the additional include paths. When ITK is built with ITK_USE_SYSTEM_DCMTK=ON the system-installed DCMTK headers are already on the default search path, so the block is gated behind NOT ITK_USE_SYSTEM_DCMTK.
Add a test that exercises every well-formed VR-typed accessor on itk::DCMTKFileReader against the existing fixture Input/DicomSeries/Image0075.dcm: GetElementDA StudyDate GetElementTM StudyTime GetElementPN PatientName GetElementCS PatientSex, PhotometricInterpretation GetElementDS PatientWeight GetElementIS EchoNumber GetElementFL AcquisitionDuration (private GE tag) GetElementSL LocsPer3DSlab (private GE tag) GetElementUS SamplesPerPixel, Rows, BitsAllocated GetElementUI ScannerStudyEntityUID Expected values were verified out-of-band with pydicom against the on-disk fixture before authoring, fixing several wrong-by-transcription expectations from the original PR InsightSoftwareConsortium#2796 (e.g. AcquisitionDuration was recorded there as 1304791694, the integer reinterpretation of the float bit pattern; the actual scanner-reported FL value is 414273984.0). Each tag is asserted in-process via ITK_TEST_EXPECT_EQUAL; the test also writes one tag-value-per-line to argv[2] for diagnostic inspection. The in-process assertions are the source of truth for correctness, so no --compare baseline file is needed. The fixture is the single slice Image0075.dcm rather than the DicomSeries regex; the test reads exactly one file via SetFileName, not a series. Two further test invocations proposed by PR InsightSoftwareConsortium#2796 (against RGBDicomSeries/raw-RGB.dcm and RTDoseDicomSeries/32bitDoseRTImage.dcm) are dropped for now because those fixtures were never published to ITKTestingData. Supersedes PR InsightSoftwareConsortium#2796. Co-Authored-By: jhlegarreta <jon.haitz.legarreta@gmail.com>
hjmjohnson
force-pushed
the
fix-pr2796-takeover
branch
from
ffeeaf2 to
365b2ba
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
itkDCMTKGetDicomTagsTestexercising every well-formed VR-typed accessor onitk::DCMTKFileReaderagainstInput/DicomSeries/Image0075.dcm, and fixes the include-path issue (#3820) that has blocked DCMTK-reader test coverage for over three years. Supersedes #2796.Two-commit structure
COMP: Expose DCMTK headers to ITKIODCMTKTestDriver (issue #3820)Modules/IO/DCMTK/test/CMakeLists.txt)target_include_directories()adding the DCMTK ExternalProject's per-library include dirs and the generated config dir to the test driver target only. Public DEPENDS surface of the IODCMTK module unchanged. Gated onNOT ITK_USE_SYSTEM_DCMTK.ENH: Add itkDCMTKGetDicomTagsTest covering DCMTKFileReader::GetElement*Co-Authored-By: jhlegarreta.Coverage map
The test exercises:
GetElementDAGetElementTMGetElementPNGetElementCSGetElementDSGetElementISGetElementFLGetElementSLGetElementUSGetElementCSGetElementUSGetElementUSGetElementUIExpected values were verified out-of-band with pydicom against the on-disk fixture before authoring. Each assertion uses
ITK_TEST_EXPECT_EQUALso a failure surfaces with a precise diff; the test also writes one tag-value-per-line to argv[2] for diagnostic inspection. No--comparebaseline file (the in-process assertions are the source of truth).What this fixes vs. PR #2796's catalogued problems
unsigned short = "????"compile errorusTargetIOPixelEnummissingitk::qualifierGetImagePixelTypetest per @malaterre's earlier feedback that the method is questionable)dcmtk/dcmdata/dcdict.hnot found (#3820)1304791694(wrong AcquisitionDuration)414273984.0via pydicomunsigned short(wrong VR)CSstring"MONOCHROME2"8(wrong)16, fixeditk_add_testnames in PR #2796argv[1]consuming a comma-listDATA{REGEX:...}WIP:title prefixLocal verification
Built
ITKIODCMTKTestDriveragainst a freshbuild-dcmtk/(withModule_ITKIODCMTK=ON,BUILD_SHARED_LIBS=ON, Release):pre-commit run --all-filesexits 0 against the pushed HEAD.PR Checklist