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

ENH: Fix dicom reading slices #4955

Conversation

hjmjohnson
Copy link
Member

Dicom files that do not have explicit "SliceThickness" listed can still be used. The actual "spacing between slices" is computed from the stack of slices when multiple dicoms are provided such that the initial setting of the "SliceThickness" placeholder is not used. (NOTE: SliceThickness is used when a single slice is read into a 3D image).

This fixes an issue when a stack of 124 dicom slices does not have "SliceThickness" fields, but does have "ImagePositionPatient" for each slice.

The previous implementation would throw an exception prior to getting to the code that explicitly computes the slice space from the "ImagePositionPatient" values.

The new code now allows ITK to make 3D nifti volumes that are compatible with those created by the dcm2niix package.

PR Checklist

The AlmostEquals function fails for slices that are only 1e-15 different
for 1.5mm slices.

The DefaultImageCoordinateTolerance has been determined to be 1e-6.

Using the global default tolerance provides more meaningful default
behaviors.
When reading individual 2D dicom slices, the slice thickness may not be
known at each slice reading.  If set to spacing of 0 (the default) an
exception is thrown before the inter-slice distance can be computed.

Catch this case and set a temporary inter-slice thickness.
@hjmjohnson hjmjohnson added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:IO Issues affecting the IO module labels Nov 15, 2024
@hjmjohnson hjmjohnson added this to the ITK 6.0 Beta 1 milestone Nov 15, 2024
@hjmjohnson hjmjohnson self-assigned this Nov 15, 2024
@hjmjohnson hjmjohnson requested a review from dzenanz November 15, 2024 16:32
@github-actions github-actions bot added type:Enhancement Improvement of existing methods or implementation and removed type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Nov 15, 2024
@hjmjohnson
Copy link
Member Author

Local mac testing:

100% tests passed, 0 tests failed out of 3011

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.

The Windows libtiff errors seem to be unrelated:

itktiff-6.0.lib(tif_luv.c.obj) : error LNK2005: __ucrt_int_to_float already defined in itktiff-6.0.lib(tif_aux.c.obj)
itktiff-6.0.lib(tif_color.c.obj) : error LNK2005: __ucrt_int_to_float already defined in itktiff-6.0.lib(tif_aux.c.obj)
bin\ITKAnisotropicSmoothingTestDriver.exe : fatal error LNK1169: one or more multiply defined symbols found

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Maybe the comment style can be improved ?

Not sure if we should say inter-slice or interslice.

I know this is hard, but can a test be added that demonstrates the enhancement ?

// file. interslice distances can be computed after all slices are read.
// set to non-zero value here to avoid prematurely throwing an exception
// before the interslice thickness can be computed.
const auto abs_spacing_2 = std::abs(sp[2]); // Spacing may be negative at this point, will be fixed below
Copy link
Member

Choose a reason for hiding this comment

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

A 2D DICOM slice

file. Inter-slice distances (leave a single white space after the period)

Set to non-zero value

before the inter-slice

@hjmjohnson
Copy link
Member Author

@jhlegarreta I appreciate your comments, but I don't have the bandwidth to make this perfect. I'm going to add it as is.

@hjmjohnson hjmjohnson merged commit 296bed6 into InsightSoftwareConsortium:master Nov 17, 2024
13 of 17 checks passed
@hjmjohnson hjmjohnson deleted the fix-dicom-reading-slices branch November 17, 2024 20:26
@seanm
Copy link
Contributor

seanm commented Nov 19, 2024

Is this at all related to #4794 ?

@seanm
Copy link
Contributor

seanm commented Nov 20, 2024

I just tracked down 916a40c as the cause of one of our app's unit test failures.

With the new less-strict tolerance check, ITK_non_uniform_sampling_deviation is no longer added to the metadata dictionary, and our test was expecting it to be there.

We use the value of ITK_non_uniform_sampling_deviation to decide for ourselves if it is above/below an acceptable tolerance. But 916a40c has now hardcoded a tolerance, making it impossible for us to choose ourselves.

It's not clear to me from the commit message what bug this commit was fixing... Yes, AlmostEquals() will return false for "slices that are only 1e-15 different for 1.5mm slices", but so what? Then ITK_non_uniform_sampling_deviation will contain a tiny number and callers can decide if they care. They can also just ignore ITK_non_uniform_sampling_deviation.

Where does this magic 1e-6 (DefaultImageCoordinateTolerance) number come from? What are its units? millimetres? metres? inches? How can we assume this number is appropriate for every kind of DICOM file under the sun?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:IO Issues affecting the IO module type:Enhancement Improvement of existing methods or implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants