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

Update gdcm #4647

Merged
merged 2 commits into from
May 6, 2024
Merged

Conversation

hjmjohnson
Copy link
Member

Release 3.0.24
COMP: Fix for gcc13.2 compiler test failures
Another pass for SIEMENS/MOSAIC
Check for Secondary Capture spacing following DICOM Part 3 Sect A.8.1.3

PR Checklist

GDCM Upstream and others added 2 commits May 5, 2024 07:49
Code extracted from:

    https://github.com/malaterre/GDCM.git

at commit 2eaae2091ed90566597c34886dd17d639c1ba8d5 (release).
* upstream-GDCM:
  GDCM 2024-05-03 (2eaae209)
@hjmjohnson hjmjohnson added the area:ThirdParty Issues affecting the ThirdParty module label May 5, 2024
@hjmjohnson hjmjohnson added this to the ITK 5.4.0 milestone May 5, 2024
@hjmjohnson hjmjohnson requested a review from malaterre May 5, 2024 13:03
@hjmjohnson hjmjohnson self-assigned this May 5, 2024
@github-actions github-actions bot added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label May 5, 2024
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

@hjmjohnson thank you!

@thewtex thewtex merged commit 4926c2f into InsightSoftwareConsortium:master May 6, 2024
14 of 15 checks passed
@seanm
Copy link
Contributor

seanm commented May 9, 2024

This caused another behaviour change in our application's test suite...

We have a test that reads a single DICOM file (just one slice) with GDCMImageIO. With the resulting itk::Image we call GetSpacing() and now we get 5.0 for the z spacing instead of the 1.0 we got before with PR.

Since it's just one slice, I'm not really sure if 1 or 5 is the right answer.

I've built GDCM master, and gdcminfo also gives 1, so I'm not sure where the 5 is coming from...

@seanm
Copy link
Contributor

seanm commented May 15, 2024

@thewtex @malaterre @hjmjohnson so we looked at this more... it seems the behaviour has only changed when there is just 1 DICOM slice.

So before this PR, with either 1 or 2 files in the folder, we get:

spacing: {0.661468, 0.661468, 1.0}

After this PR, with 1 file we get:

spacing: {0.661468, 0.661468, 5.0}

but with 2 files we get:

spacing: {0.661468, 0.661468, 1.0}

Is this a deliberate change? BTW the file we are using is D_CLUNIE_CT1_J2KR.dcm, which is in the GDCM test suite.

@thewtex
Copy link
Member

thewtex commented Jun 3, 2024

@seanm there is a flag that needs to be passed to gdcminfo to get the additional metadata, --scipm

@seanm
Copy link
Contributor

seanm commented Jun 3, 2024

@thewtex thanks for your reply! So without --scipm:

Spacing: (0.661468,0.661468,1)

and with --scipm:

Spacing: (0.661468,0.661468,5)

Interesting. OK, so this explains differences using gdcminfo I guess.

But we're using ITK. We use itk::ImageSeriesReader / itk::GDCMImageIO and the result is different between ITK 5.3 and 5.4 (for the case of 1 file in the folder). Perhaps some sample code will help, see attached, it's only 70 lines.
DICOMReader-Single_vs_MultiFile-Series.zip

What we're trying to understand is if this behaviour change between ITK 5.3 and 5.4 is deliberate / correct / buggy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ThirdParty Issues affecting the ThirdParty module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants