WIP: BUG: The Z spacing was set using thickness

[hjmjohnson]

This addresses https://issues.itk.org/jira/browse/ITK-3527

According Dicom standard (DICOM PS3.6 2016b - Data Dictionary)
(0028, 0030) indicates physical X,Y spacing inside a slice;
(0018, 0088) indicates physical Z spacing between slices;
which above are also consistent with Dcom2iix software.

In current code, the zSpace is get from (0x0018,0x0050) which
actually is the thickness from which the signal is created,
not the separation of signal samplings.

This patch set resolves the problem by updating the Z get
spacing method by using (0018, 0088) when it is available.

If (0018, 0088) is not available, then the code reverts to
the previous behavior of using (0x0018,0x0050).

[hjmjohnson]

http://review.source.kitware.com/#/c/22094/

Hans J. Johnson
Uploaded patch set 1.Feb 22, 2017

Hans J. Johnson
Uploaded patch set 2.Feb 22, 2017

Kitware Build Robot
Patch Set 1: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22094-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 22, 2017

Francois Budin
Patch Set 2: Code-Review+1Feb 22, 2017

Hui Xie
Patch Set 2: Code-Review+1 Verified+1 Good. Verified. thank you for helping me committing.Feb 22, 2017

Kitware Build Robot
Patch Set 2: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22094-2&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 22, 2017

Hui Xie
Uploaded patch set 3.Feb 23, 2017

Kitware Build Robot
Patch Set 3: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22094-3&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 23, 2017

Matt McCormick
Patch Set 3: Code-Review-1 In GDCM, I believe we use the difference between Image Position Patient between the first two slices, which should probably be used here, too.Feb 24, 2017

Francois Budin
Patch Set 3: To keep track of the on-going conversation, here are the link to the relevant GDCM code that I included in the email I sent: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/GDCM/src/gdcm/Source/MediaStorageAndFileFormat/gdcmImageHelper.cxx#L163-L279Feb 24, 2017

Hui Xie
Patch Set 3: It looks that the GDCM code still not handle the Mosaic format of Siemens. Does anyone know whether the GDCM code passed the test case of Mosaic of Siemens?Feb 25, 2017

Hui Xie
Patch Set 3: Thank all of your contribution on this bug fixing. Basing on your contribution, I summarize my initial thinking below: 1 use (0020, 0032) Image Position (Patient) of type 1C is a better choice than (0018, 0088) of type 3; 2 when (0020,0032) is absent as the prerequisite (0008,9007) and (0008,9206) are not met, we may try to get (0018, 0088); if failed again, throw out an exception; 3 current itk::DCMTKFileReader is a container class of one dicm header file, which can not includes 2 frames, is not a good place to contain GetZSpace() method; we need to put it into a higher class; Where is this higher class? Welcome your suggestion. 4 GetZSpace should have 2 input parameter: 2 adjacent frame header files; 5 2 adjacent frame headers should filter from general slice interleave, volume interleave, or mosaic format, which needs extra work to adapt; 6 GetZSpace should be low-cost simple property-getting method, how to reduce the extra cost of get 2 adjacent frame headers is worth further designing and considering;Feb 25, 2017

Matt McCormick
Patch Set 3: +1 for better typed DICOM <-> ITK conversion. That path was started here: https://github.com/KitwareMedical/ITKDICOM but it will require some work.Feb 26, 2017

Hui Xie
Patch Set 3: A Technical Discussion Summary of DCMTKFileReader::GetSpacing() Feb 28th, 2017 Yesterday My professor Johnson discussed with me about the further work of GetSpacing(). As this spring semester I have other tasks, I will not have time enough to contribute on the reconstruction of GetSpacing. For the reconstruction of GetSpacing(), basing from the important and invaluable contribution from Steve, my immature thinking is: 1 referencing GDCM code and using the difference between (0020, 0032) Image Position (Patient) of adjacent 2 frames is relatedly simply in concept; 2 Under the hood of the above idea, how to find the adjacent 2 frames is the real challenge and needs a lot of work, as it involves to analyze various vendors’ Dicom format, for example, Siemens’ mosaic, slice interleave, or volume interleave etc; 3 In DWIConvert module, this reconstruction involves business logic reconstruction of DWIDICOMConverterBase and its derived classes; In other words, we need to redesign the order of getting Dicom properties data. 4 In 3D Slicer, it also involves an architecture design about how to design various Dicom vendors’ plugins. If we consider the plugins only for GetSpacing(), it is a little myopic. We need to think it in a broader view of getting Dicom data logic to design the Dicom plugins. Like Steve has said, DWI and FMR also need to consider together. In the communication with Steve, I always feel your suggestions are very helpful and encouraging. I like your style very much. Steve, you are a great leader. I believe that a lot of people like to cooperate and follow with you. If this summer break I have time, I would like to continue to cooperate with you on possible work. Now I summarize the previous technical discussion in the historical order for someone to refer in the future. Some of important discussion below are in email form which was not embedded in the web e-flow. I copied them into this document for future reference, which is precious asset for the betterment of ITK. Issue was submitted and some tries was discussing: BRAINSia/BRAINSTools#334 ITK Code modification discussion on http://review.source.kitware.com/#/c/22094/ Felix Navarro on Feb 07th, 2017: Dear developers, I've found an issue regarding slice thickness and slice separation using DWIconvert. I've posted it on Feb 7th on slicer user's forum (link) I copy my analysis here: This is the report: I'm testing the conversion from DICOM to nifti using DWIConvert. You can download these dicom images from the TCIA, Ivy GAP collection, patient W2, series 500 using the TCIA explorer module or using the GUI provided by TCIA's web. Dicom tags related to geometrical aspects: /(0018,0050) Slice Thickness 5.000000 (0018,0088) Spacing Between Slices 6.000000 (0020,0037) Image Orientation (Patient) 0.996604\0.0823383\0-0.0823383\0.996604\0/ This is the conversion sentence: c:>slicer --launch DWIConvert --conversionMode DicomToFSL --outputVolume ADCSlicer.nii --inputDicomDirectory C:\Temp\W2_500_DICOM --outputDirectory ./ --echo >> ConversionSlicer.log This is the important content of ConversionSlicer.log file: /=================== this->m_SlicesPerVolume:25 Dicom images are ordered in a volume interleaving way. ImageOrientationPatient (0020:0037): LPS Orientation Matrix 0.996604 -0.0823383 0 0.0823383 0.996604 -0 0 0 0.999999 this->m_SpacingMatrix 0.9375 0 0 0 0.9375 0 0 0 5 NRRDSpaceDirection 0.934316 -0.0771922 0 0.0771922 0.934316 0 0 0 5 Slice 0: -109.284 -161.358 -82.5368 Slice 1: -109.284 -161.358 -76.5368 Slice order is IS Number of Slices: 25 Number of Volume: 1 Number of Slices in each volume: 25 B-value: 1000; diffusion direction: 0, 0, 0 Scale Factor for Multiple BValues: 0 -- sqrt( 1000 / 1000 ) = 1/ The first thing that catches my attention is that the pixel spacing is 5 mm instead of 6 mm which is the true IS spacing among the centers of voxels in adjacent slices. I have converted these files using dcm2niix also: dcm2niix -f ADCdcm2niix -o ./ C:\Temp\W2_500_DICOM which reports: / .... Slice thickness (mm): 5.000000 Slice spacing (mm): 6.000000 Image columns: 256 Image rows: 256 Voxel size x (mm): 0.9375 Voxel size y (mm): 0.9375 .... / If you load both volumes on slicer the volume converted with DWIConvert appears upside down and you cannot superpose them because regions do not match. If your make the conversion using DWIConvert with two steps,DicomToNrrd and then NrrdToFSL, then you get the volume correctly oriented but clearly compressed. Changing the z pixel spacing from 5 to 6 mm solves this issue. Taking this Z pixel spacing mismatch into account I draw a labelmap using that 2-step DWIConverted volume as master volume, then I save that labelmap using nii format and try to map it over the other one using the information provided in the saved file with a R script. Qaternions saved (b,c,d): (0,0, -0.9991508) Qform (ROI over 2 steps DWIconverted volume) : [,1] [,2] [,3] [,4] [1,] -0.93431626 0.07719193 0 109.2840 [2,] -0.07719193 -0.93431626 0 161.3580 [3,] 0.00000000 0.00000000 5 -82.5368 [4,] 0.00000000 0.00000000 0 1.0000 If I fix the position [3,3] changing its value from 5 to 6, the modified positions of the ROI using qform(ADCdcm2niix)^-1*qform(ROIover2StepsDWIConverted) match perfectly over the volume converted with dcm2niix. I'm going to perform further investigations, if I find something interesting I'll inform you. Steve Pieper on Feb 7th, 2017: See this related bug report that includes a link to phantom data: http://na-mic.org/Mantis/view.php?id=3867 Also see slicer-users thread here http://massmail.spl.harvard.edu/public-archives/slicer-users/2017/011619.html Hui Xie on Feb 9th, 2017: Hi, Felix, Thank you for pointing out this bug. I initially located this bug: In ITKv4/Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx Line 1350 to Line 1355: if(subSequence.GetElementDS(0x0028,0x0030,2,_spacing,false) == EXIT_SUCCESS && subSequence.GetElementDS(0x0018,0x0050,1,&_spacing[2]) == EXIT_SUCCESS) { spacing[0] = _spacing[1]; spacing[1] = _spacing[0]; spacing[2] = _spacing[2]; Maybe 0x0050 above should be 0088. Or maybe it is about space direction conversion. I need test with your original data to make sure. I went to https://public.cancerimagingarchive.net/ncia/externalLinks.jsf?collectionName=IvyGAP to try to get your data, but I can not find the series 500 data you mentioned. Would you please give me more in detail about where to get your test data? Or you may share the test data with me through Dropbox by my gmail: SheenXH@gmail.com. Thanks, have a nice weekend, Hui Xie Steve Pieper on Feb 9th, 2017: Hi Hui - Be sure to always use the differences between Image Position Patient of adjacent slices to calculate the third spacing value and don't rely on the Spacing Between Slices (0018,0088, which is optional and often missing or incorrect). Here's a discussion from the ITK mailing list that may help: https://itk.org/pipermail/insight-users/2008-November/027903.html Also here's some example code from slicer that calculates the correct spacing values and then confirms that each adjacent pair of slices is within espilon of that spacing: https://github.com/Slicer/Slicer/blob/master/Modules/Scripted/DICOMPlugins/DICOMScalarVolumePlugin.py#L195-L253 It's important to do this because itk::Image, nrrd, and nifti all require uniform slice spacing with no missing slices whereas a dicom series may be missing a slice (e.g. if a file is deleted by mistake). Hope that helps, Steve Hans Johnson on Feb 9th, 2017: I believe that the spacing being created is done deep in ITK itself. DWIConvert does not directly compute this value, but rather uses the value returned by ITK. If there is a bug, it is likely in ITK itself. Steve Pieper on Feb 9th, 2017: Right - the issue is in the itk dcmtk image io which is used in DWIConvert. Hui Xie on Feb 9th, 2017: Thank you very much, Pieper. After I read the link https://itk.org/pipermail/insight-users/2008-November/027903.html you gave, I find that 0018,0088 is a "big hollow" full of ambiguity and misinterpretation. This is why the int DCMTKFileReader::GetSpacing(double * const spacing) const writes so coiling like below. I plan not to change ITK code, let me to have a look whether there is another way to help Felix solve this bug. Thank Steve Pieper very much. I will follow your suggestion of "use the differences between Image Position Patient of adjacent slices to calculate the third spacing value" to modify ITK code: int DCMTKFileReader::GetSpacing(double * const spacing) const Hui Xie on Feb 18th, 2017: Today I analyzed the latest code of Dcm2niix from Github: 1 it directly uses (0028, 0030) to get physical X,Y spacing inside a slice; 2 it directly uses(0018, 0088) to get physical Z spacing between slices; 3 it directly uses (0018, 0050) go get slice thickness; The "directly"s above mean there is no other logic judgement to get these values, just plainly do it. It looks that Dcm2niix is not so complex comparing with ITK::DCMTKFileReader::GetSpacing(). Can we believe the judgement of Dcm2niix? Current ITK:: DCMTKFileReader has not getThickness() method, BRAINSTools just directly use zSpace as thickness. Do we need to add a getThickness() method in ITK::DCMTKFileReader? Hans Johnson on Feb 18th,2017: Thickness is not important to us. It is not a relevant measure for our DWI work. Slice spacing is important, but thickness is not. Hui Xie on Feb 21st, 2017: This morning, I analyzed the header file structure of FSL(NifTi). In the 348-bytes header of NifTi, there is no place to store thickness information. And 348 bytes which is already occupied compactly leave no space for private information extension. In another words, you will loss thickness information from Dicom to FSL, or from Nrrd to FSL. I will not store thickness into or read thickness from FSL file in our DWIConvert. Hans Johnson on Feb 22nd, 2017: Change subject: BUG: The Z spacing was set using thickness ...................................................................... BUG: The Z spacing was set using thickness This addresses https://issues.itk.org/jira/browse/ITK-3527 According Dicom standard (DICOM PS3.6 2016b - Data Dictionary) (0028, 0030) indicates physical X,Y spacing inside a slice; (0018, 0088) indicates physical Z spacing between slices; which above are also consistent with Dcom2iix software. In current code, the zSpace is get from (0x0018,0x0050) which actually is the thickness from which the signal is created, not the separation of signal samplings. This patch set resolves the problem by updating the Z get spacing method by using (0018, 0088) when it is available. If (0018, 0088) is not available, then the code reverts to the previous behavior of using (0x0018,0x0050). Change-Id: I1111c55dd9e15444411e31bd6f2f1e68b7cf7618 Steve Pieper on Feb 22nd, 2017: Hi Hui, Hans, Francois - I don't think the current commit completely solves the problem. 1) SpacingBetweenSlices (0018,0088) is type 3, meaning it's optional in the standard. I think in practice it is rarely given in files. 2) using the SliceThickness (0018,0050) is not a good fallback because it can be completely unrelated to spacing. There are known cases where it is innaccurate. 3) The correct thing to do is calculate the spacing as the distance between adjacent slices using ImagePositionPatient because those fields are mandatory and are almost guaranteed to be correct in practice. In the emails about this topic I pointed to example code that implements the better solution. 4) I would suggest that any code that says 'punt' or picks an arbitrary number like 1.0 should be removed and an exception thrown instead. I don't like the idea that ITK will silently read invalid data. I we want a 'lax' mode that that should be a flag that the user explicitly sets. But the way I tried to log in through review.source.kitware.com but clicking the login link results in a page that simply says "Forbidden". Thanks for working on this, Steve Hui Xie on Feb 22nd, 2017: Hi, everyone, Good afternoon. Calculating z spacing as the distance between adjacent slices using ImagePositionPatient is every good idea in theory. I tried the above idea but failed. There are reason below: 1 Siemens Dicom has mosaic format which includes a collection of 2D slices in one slice. And Siemens has the private CSA header for each slice. It is difficult to get origin coordinates of adjacent slices except after we re-arrange the whole slices into normal order; 2 Philips has 2 cases: one header for each volume or one header for each slice; it leads use different method to get the original coordinates of adjacent slices; 3 In other words, Calculating z spacing as the distance between adjacent slices using ImagePositionPatient needs to consider the different scanner manufactory; The nonstandard of Dicom leads difficulties above to get the original coordinates of adjacent slices. And I reference the implement of Dcm2niix software. It just simply, directly use (0018, 0088) to get the zSpace, and it looks pretty good in many cases. Therefore I modified the ITK::itkDicomFileReader::GetSpace() to first get (0018, 0088); if it fail, then try to get(0018, 0050) instead. If punt of 1 is not good, which is the historical implement of GetSpace(), I think I can modify it to throw out an exception. Steve Pieper on Feb 23rd, 2017: Hi Hui - Agreed - this is complicated in many ways. Definitely the code predates your involvement so I certainly don't blame you for the 'punt' comments! It's great if we can work together to implement good behavior in as many real world cases as possible. I also agree that there is a lot of vendor-specific interpretation required, particularly for DWI dicom. I'm adding Andrey Fedorov to the cc list for this thread because for our QIICR project we are also working with DWI acquisitions and have been discussing how best to translate legacy acquisitions into a standard forms. A few comments: * Since the itkDCMTKFileReader can be used for any kind of DICOM, it should have robust behavior at estimating the spacing between slices. As far as I know the code from Slicer and the GDCM reader represent the best practices for determining the spacing for a wide range of real world cases. * Vendor-specific DWI may come in many forms and these require custom overrides of the spacing for the reasons you discussed. I think the DWIConvert code may be the right place to handle purely-DWI related calculations, but since multiframe and mosaic datasets may come up in fMRI or other acquisitions it probably makes the most sense to factor out the code that handles these cases and put it directly in the itkDCMKTFileReader. * Ultimately we may have improved functionality in DCMTK itself for handling some of these conversions. It would be good to think about what API would be most helpful and work with the DCMTK developer community to see how it could be implemented. * Regarding the 'punt' comments in the code, yes, it would be great to convert those to exceptions so the application can warn the user of corrupt or otherwise non-standard data. Thanks again for your work on this, Steve Hui Xie on Feb 23rd, 2017: Hi, Steve, Good evening. Your consideration is from the comprehensive requirements of medical image reality, and for excellence of our software system. I like this. OK. I will follow your suggestion to continue work on this to get the best solution. Thank you very much and best regards, Matt McCormick on Feb 24th, 2017: In GDCM, I believe we use the difference between Image Position Patient between the first two slices, which should probably be used here, too. Francois Budin on Feb 24th, 2017: To keep track of the on-going conversation, here are the link to the relevant GDCM code that I included in the email I sent: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/GDCM/src/gdcm/Source/MediaStorageAndFileFormat/gdcmImageHelper.cxx#L163-L279 Hui Xie on Feb 25th, 2017: It looks that the GDCM code still not handle the Mosaic format of Siemens. Does anyone know whether the GDCM code passed the test case of Mosaic of Siemens? Thank all of your contribution on this bug fixing. Basing on your contribution, I summarize my initial thinking below: 1 use (0020, 0032) Image Position (Patient) of type 1C is a better choice than (0018, 0088) of type 3; 2 when (0020,0032) is absent as the prerequisite (0008,9007) and (0008,9206) are not met, we may try to get (0018, 0088); if failed again, throw out an exception; 3 current itk::DCMTKFileReader is a container class of one dicm header file, which can not includes 2 frames, is not a good place to contain GetZSpace() method; we need to put it into a higher class; Where is this higher class? Welcome your suggestion. 4 GetZSpace should have 2 input parameter: 2 adjacent frame header files; 5 2 adjacent frame headers should filter from general slice interleave, volume interleave, or mosaic format, which needs extra work to adapt; 6 GetZSpace should be low-cost simple property-getting method, how to reduce the extra cost of get 2 adjacent frame headers is worth further designing and considering; Steve Pieper on Feb 25th, 2017: I'm pretty sure that gdcm never handled Siemens mosaic format. Hi Hui - Thanks for your careful thoughts on this issue. It will be good to get a nice architecture that will help all ITK users work with DICOM files more carefully. Your question in point 3 about the need for a higher class is a particularly important one. The mapping from a collection of DICOM image instances to itk::Image instances is not always one-to-one. I agree with you that a single frame image cannot properly know it's Z spacing; this can only be known calculated when there are multiple frames. It might be best to identify very specific use cases that will be supported and then throw exceptions if the constraints of those cases are not me. The following is a suggestion based on what is implemented in Slicer (in a very slicer-specific way, and not in native ITK code). 1) DICOM volumetric series --> itk::Image (could be done in the DCMTK based reader) 1a) given a single filename of a dicom file, search the rest of the directory for files with the same SeriesInstanceUID (this is like itkGDCMImageSeriesFileNames) 1b) for a set of DICOM files of the same series, check that the geometry and other properties meet the assumptions of an itk::Image, meaning same pixel type, z direction vector is orthogonal to the scan plane, pixel spacings are uniform through entire volume, etc. Basically any field that is an instance variable of itk::Image needs to be true for the entire PixelData. 1c) the mapping from DICOM frames to itk may have multiple interpretations, such as a vector PixelType for one image or a vector of images with a scalar pixel type (or combinations). It should be possible for users to provide application-specific plugins that allow custom interpretations. 2) DICOM DWI --> nrrd or other DWI format (could be done in DWIConvert) 2a) provide plugins to 1c that know about specific DWI encodings from various vendors and scanner types into a unified generic representation of DWI of the appropriate image and pixel types. 2b) provide translations from the generic DWI into any of the supported export types (nrrd, nifti, etc). Hui Xie on Feb 25th, 2017: Hi, Steve, Good afternoon on Saturday. Thank you for your helpful suggestions in detail. 1 Your "given a single filename of a dicom file, search the rest of the directory for files with the same SeriesInstanceUID " is a wonderful and reality idea, although it lightly violates the object-oriented principle; I will consider this idea in my implementation. 2 Yes. I agree with you that checking the uniformity of zSpacing in entire volume is very important; this should be considered in our implementation; 3 Current major challenge is about how to get correct adjacent frames because of the various vendors; Plugin is a good choice for Slicer. For DWIConverter, this is a inheritance class design problem; 4 I am thinking another problem: Does DWIConverter::GetSpacing need to execute before rearranging pixel buffer data (this is status quo), or it may execute after rearranging pixel buffer data? The answer to this problem will lead a light-weight, or heavy-weight ITK::GetZSpacing(). If DWIConverter::GetSpacing may execute after rearranging pixel buffer data, we will have clear frame order, then it will be very easy to get z Spacing from (0028, 0032) from adjacent frames. It think it is possible in theory; 5 I need some time to research in detail frame order mechanism of Siemens Mosaic format and Phillips. By the way, about my time on this work: I estimate that I only have 10 hours per week to invest in this work, as on April 22nd I will have a PhD qualification exam, and before it I need about one month to prepare it; And in March I also need to finish another ITK super resolution project implementation. Thank you very much, have a nice new week, Hui Xie Matt McCormick on Feb 26th, 2017: for better typed DICOM <-> ITK conversion. That path was started here: https://github.com/KitwareMedical/ITKDICOM but it will require some work. Hui Xie on Feb 27th, 2017: As the modification of itk::itkDCMTKFileReader::GetSpacing for getting correct zSpace using use (0020, 0032) Image Position (Patient) needs to handle the frame order of various MR vendors, e.g. Siemens Mosaic format, this work, I guess, needs a lot of time. Maybe one month or 3 month, I don't know. Therefore, I implemented temporarily a bypass solution inside BRAINSTools named int DWIDICOMConverterBase::getDicomSpacing(double * const spacing) const.Feb 28, 2017

Hans J. Johnson
Uploaded patch set 4: Patch Set 3 was rebased.Jul 11, 2017

Kitware Build Robot
Patch Set 4: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22094-4&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Jul 11, 2017

Matt McCormick
Patch Set 4: Follow-ups to this patch will also address: https://issues.itk.org/jira/browse/ITK-3554Jul 19, 2017

Hans J. Johnson
Uploaded patch set 5: Patch Set 4 was rebased.Feb 4, 2018

Hans J. Johnson
Patch Set 4: https://insightsoftwareconsortium.atlassian.net/browse/ITK-3554Feb 4, 2018

Kitware Build Robot
Patch Set 5: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22094-5&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 4, 2018

[hjmjohnson]

Accidently deleted/closed branch when the script for moving from Gerrit went haywire.

[malaterre]

@dzenanz requested my review. I believe the code should be mostly fine on real world system, and is actually going in the right direction since using (0018,0050, Slice Thickness) is a major flaw. But since I am strong opponent of using the (0018, 0088, Spacing Between Slices) in code base, I will vote against this PR. At a minimum I would suggest removing legacy mechanism of using Slice Thickness, the proper behavior for recomputing the Reconstruction Interval is described at:

https://stackoverflow.com/questions/37730772/get-distance-between-slices-in-dicom/41848360#41848360

or else somewhere in the itk::GDCMImageIO codebase.

[hjmjohnson]

@malaterre Is there any chance that you would be willing to fix this PR? It could really use an expert in the area to get it right. We have been hacking as sub-par solutions for a long time.

I'd like to stay focused on other ITK improvements.

Thank you for your consideration. Otherwise it will likely linger for a while longer.

[malaterre]

@hjmjohnson As mentioned above, I find acceptable the solution where you simply remove the legacy code path with Slice Thickness.

Eg:

if(GetElementDS<double>(0x0018,0x0088,1,&_spacing[2], false) == EXIT_SUCCESS
       || GetElementDS<double>(0x0018,0x0050,1,&_spacing[2],false) == EXIT_SUCCESS)

should instead be simply:

if(GetElementDS<double>(0x0018,0x0088,1,&_spacing[2], false) == EXIT_SUCCESS)

That's an acceptable solution for now.

[dzenanz]

I will edit the PR per your suggestion @malaterre

[hjmjohnson]

see #243