Update GDCM to 2018-11-05 release branch

[thewtex]

No description provided.

[malaterre]

Yay ! Thanks for the update. Looks good.

[malaterre]

Good to go.

[fedorov]

Would it be possible to leave a comment explaining the role of interSlicer here? Just from looking at the code, and the definition of SpacingBetweenSlices ("The spacing is measured from the center-to-center of each slice."), I cannot understand the logic.

[malaterre]

This was submited by @tbaudier see explanation at:

• Take into account slice thickness in RetrieveSOPInstanceUIDFromZPosition malaterre/GDCM#57

[fedorov]

@malaterre unrelated to this PR, but I am just curious, why don't you have #defines for DICOM attribute names, and always refer to them by tags (contrary to the conventions in DCMTK, for example). This practice strikes me as an approach that at the very least makes developers' life more complicated by requiring to always look up attribute names, and at extreme can cause errors by mis-typing tag numbers.

[malaterre]

Well we do have the mechanism since GDCM 2.4 (see Source/DataDictionary/gdcmTagKeywords.h).
For example, see some (rare) usage:

• https://github.com/malaterre/GDCM/blob/master/Source/MediaStorageAndFileFormat/gdcmEmptyMaskGenerator.cxx#L76-L77

So the only lame excuses I can write quickly:

1. Code using those keywords wont be compatible with GDCM 2.2
2. A totally made up excuse is that the header file is large, and I do not like waiting for compilation to finish
3. I should not do finger pointing, but a very well known and respected participant of WG6 promised that Keyword as defined in Part6 would never changed once introduced. However it already happen at least twice (top of my head) that keywords were updated (some kind of minor typos). This means code is fragile to DICOM editor workgroup change of mind.

Let me know if you want me to build other lame excuses not to start using those keywords in GDCM and make the codebase slightly easier to read ;)

[fedorov]

@malaterre thanks, that makes sense. I am glad that I did not hear hard resistance! I also saw this practice in ITK DCMTK IO module, and it really made me cringe ... Glad to hear the use of tags is not the convention that is encouraged or set in stone, even in GDCM proper!

[thewtex]

@malaterre @fedorov @hjmjohnson thanks for the reviews and helpful discussion.

I am traveling for the Thanksgiving Holiday; after I return, I will add a test for properly reading the metadata from the mulit-frame image referenced on the Slicer Discourse, merge, and follow-up as needed on the dashboard.

[hjmjohnson]

@thewtex requested that this not be merged until after holiday break so he can add tests and have time to deal with dashboard issues.

[issakomi]

@malaterre @fedorov @hjmjohnson thanks for the reviews and helpful discussion.

I am traveling for the Thanksgiving Holiday; after I return, I will add a test for properly reading the metadata from the mulit-frame image referenced on the Slicer Discourse, merge, and follow-up as needed on the dashboard.

IMHO, it were better to wait and clarify order of frames in multi-frame images. Unfortunately this particular file is a bad example, right - left is flipped, probably worst thing what can happen in medical imaging. Probably we have to start immediately doing something against. I do validation in my app, but for community IMHO were better to do sorting in GDCM, It will not solve all problems with more complex files, so old behavior is important too to, at least for my app. My suggestion is to do make something like gdcm::SetSortMultiFrame(bool).

[fedorov]

this particular file is a bad example, right - left is flipped

@issakomi note the example in question was created by David Clunie. Also, if I use dcm2niix to convert it into NIfTI, the result lines up with the original MR series (available here for your reference) using Slicer.

If your statement is accurate, it would imply there are severe bugs somewhere in well-known open source tools developed and maintained for a long time by very experienced developers. Not saying it is impossible, but if I were you, I would want to triple-check before making such statement as yours.

[issakomi]

My point is, exactly as you've written too, we need sort frames in multi-frame. Not sure it is a bug in converter tools. If you don't want sorting, forget my post. This commit didn't change logic for multi-frames, everything is as before, only added Legacies to the list. I think, legacy multi-frames are somehow more dangerous, from my experience. May be i shall make example to prove this later, in Slicer it not possible, Slicer warns only that geometry was not processed.

[fedorov]

@issakomi my previous post was referring to your statement that the sample object is incorrect. I think it is important to understand whether this is the case, since quality of DICOM implementation is dependent on the quality and variety of data that was used to test it.

Yes, I believe sorting of the frames would be appropriate, and would be consistent with the existing practice of sorting slices stored in separate instances for the non-enhanced modalities, which I thought ITK is doing under some conditions.

[issakomi]

I have tested again particular file mf.dcm
First - i wrote a simple program to print IPP from Per-Frame Sequence
Here are results:

Frame 0 -83.2177 -123.869 159.934
Frame 1 -81.8877 -123.869 159.934
Frame 2 -80.5577 -123.869 159.934
...
...
Frame 126 84.3623 -123.869 159.934
Frame 127 85.6923 -123.869 159.934

After it i loaded classic-series you gave into Slicer:
Origin in Slicer for classic series is -85.6923 123.869 159.934
If i take into account LPS->RAS (-1,-1, 1) - last frame is origin, not first.

I don't know if it is a proof for a bug somewhere, also note In-Stack Position Number is reversed in this file, i didn't took it into account now, nobody does, AFAIK, my goal is not to search bugs in images, but prevent them, even theoretically possible.

#include <gdcmReader.h>
#include <gdcmTag.h>
#include <gdcmItem.h>
#include <gdcmSmartPointer.h>
#include <gdcmDataElement.h>
#include <gdcmDataSet.h>
#include <gdcmSequenceOfItems.h>
#include <gdcmAttribute.h>
#include <string>
#include <iostream>

int main(int argc, char ** argv)
{
	if (argc < 2) return 0;

	gdcm::Reader reader;
	reader.SetFileName(argv[1]);
	if (!reader.Read()) return 1;
	gdcm::File &file = reader.GetFile();
	gdcm::DataSet &ds = file.GetDataSet();

	const gdcm::Tag tPerFrameFunctionalGroupsSequence(0x5200,0x9230);
	if (!ds.FindDataElement(tPerFrameFunctionalGroupsSequence))
		return 1;

	const gdcm::DataElement & e =
		ds.GetDataElement(tPerFrameFunctionalGroupsSequence);
	gdcm::SmartPointer<gdcm::SequenceOfItems> sq = e.GetValueAsSQ();
    if (!(sq && sq->GetNumberOfItems()>0))
		return 1;

	for (unsigned int x = 0; x < sq->GetNumberOfItems(); x++)
	{
		gdcm::Item    & item = sq->GetItem(x+1);
		gdcm::DataSet & nds  = item.GetNestedDataSet();
		const gdcm::Tag tPlanePositionSequence(0x0020,0x9113);
		if (!nds.FindDataElement(tPlanePositionSequence))
			return 1;
		const gdcm::DataElement & e1 =
			nds.GetDataElement(tPlanePositionSequence);
		gdcm::SmartPointer<gdcm::SequenceOfItems> sq1 =
			e1.GetValueAsSQ();
		if (!(sq1 && sq1->GetNumberOfItems()>0))
			return 1;
		gdcm::Item    & item1 = sq1->GetItem(1);
		gdcm::DataSet & nds1  = item1.GetNestedDataSet();
		const gdcm::Tag tImagePositionPatient(0x0020,0x0032);
		if (!nds1.FindDataElement(tImagePositionPatient))
			return 1;
		const gdcm::DataElement & e2 =
			nds1.GetDataElement(tImagePositionPatient);
  		gdcm::Attribute<0x0020,0x0032> at;
 		at.SetFromDataElement(e2);
		std::cout
			<< "Frame " << x
			<< " " << at.GetValue(0)
			<< " " << at.GetValue(1)
			<< " " << at.GetValue(2) << std::endl;
	}
	return 0;
}

Edit:
Here is archive containing the mf.dcm file, classic series and test program, if somebody wants test.
Link

[issakomi]

Is the sample object (mf.dcm) correct or not - is a question of definition, may be standard allows, i am not sure, i'll try to clear it later.

I've written another program, it sorts frames and prints output (doesn't update data yet). So sorting is 50% done.

$ ./mfsort mf.dcm
Slice 0 --> Frame 127
Slice 1 --> Frame 126
Slice 2 --> Frame 125
Slice 3 --> Frame 124
Slice 4 --> Frame 123
Slice 5 --> Frame 122
Slice 6 --> Frame 121
Slice 7 --> Frame 120
...

Link

There are several multi-frame files in the archive, one of them, from Siemens, has the same issue, BTW.

Another prove above, IMHO, most files are Slice 0 --> Frame 0, etc. But i shall not work longer on the problem. For my project this is not required at all, i am happy with ITK/GDCM as they are. Only important were to preserve old GDCM behavior too (for more complex files -with multiple stacks, 4D, time, etc.)

Regards

Edit: Updated prog (min fix, same results, please take latest) and samples (added Philips ASL orient. file)