[2f4b1b]: Merge branch 'bug-7210-DICOMlocale'

Merged commits:

2011-03-23 14:37:31 Thomas Kilgus [c8f551]
Removed standardFileLocationStuff. Furthermore, the test is using mitk::Equal instead of a hard coded epsilon

Test still fails on the continuous client during mitk::Equal()

see http://cdash.mitk.org/testDetails.php?test=1217306&build=13455

[2691ed]: Merge branch 'bug-7210-DICOMlocaleReopened'

Merged commits:

2011-03-31 13:24:30 Thomas Kilgus [992c10]
Inserted output to debug on the dashboard

It seems as if the loaded DICOM image does not full fill the required specifications...

following error is posted as dashboard output
0.120] CoreObjectFactory c'tor
#0.120# ERROR: CreateMoreUniqueSeriesIdentifier() could not access all required DICOM tags. You are calling it wrongly. Using partial ID.
#0.120# ERROR: Error from exception: basic_string::_S_construct NULL not valid

see http://cdash.mitk.org/testDetails.php?test=1229916&build=14511

@Daniel were there recent changes in the DICOM reading process?

Yes, there were. I'll compile a version and run the test to see what's wrong.

Ok, was now able to reproduce the error. I enhanced DicomSeriesReader locally with a method to get its configuration (needed since the class has multiple #ifdefs). Would like to keep this method in the class.(In reply to comment #3)

[2691ed]: Merge branch 'bug-7210-DICOMlocaleReopened'

Merged commits:

2011-03-31 13:24:30 Thomas Kilgus [992c10]
Inserted output to debug on the dashboard

This is not really a good way to debug errors. It affects all users and half of the time people forget to remove debug output.l

Found more issues in this test case:

1. DataNodeFactory calls DicomSeriesReader in a way that makes it scan ALL files (not only DICOM) in given directory.
   • I need to check in more detail, whether the additional files are just overhead or actually processed.

2. A warning about missing tag "Slice Thickness" is issued
   • Tag IS actually missing
   • I made the routine that issued the warning more robust. It now continues processing when a single tag is missing.
   • actually the "Image Plane" DICOM module is not part of a Secondary Capture Image. However the "SC Image" module pulls in a conditional "Pixel Spacing" tag from the "Basic Pixel Spacing Calibration" macro.

3. Spacing 1.0 is produced for the mitk::Image in the end. Needs more research.

4. comp.protocols.dicom post by David Clunie:
   
   http://groups.google.com/group/comp.protocols.dicom/browse_thread/thread/55c352e70d46bf1a
   
   "(0028,0030) Pixel Spacing" is inappropriate for SC, however used sometimes "(0018,2010) Nominal Scanned Pixel Spacing" is the appropiate tag for SC images

Conclusion:

• the test image is not a 'simple' DICOM image. The test case could well work with a simple single slice CT/MR image, because it is about regarding system locales during loading.
• however, DICOM loading should make use of the pixel spacing hints that are found in the image, because actually there is the "Pixel Spacing" tag and it tells about a 0.3,0,3 pixel spacing

Steps to do:

• I'll check why the spacing is misinterpreted. If it can be fixed, it will be.
• I'll split the test into multiple cases: One with a proper CT image, one proper MR image, one SC image

Ok, I debugged into gdcmImageHelper.cxx:

• it properly recognizes the image as Secondary Capture Image Storage
• it tries to read tag (0018,2010) Nominal Scanned Pixel Spacing
• it just cannot find it

Resulting conclusion:

• GDCM makes sane assumptions, tries to correctly read an SC image
• Our test image is not an easy case but a special SC image

• Yes, it would be nice if GDCM would try to use other pixel spacing tags if the first one fails. From how I read the DICOM standard, it is not an error to have the (0028,0030) tat and not (0018,2010) in a SC image.

I suggest this solution:

• documentation of DicomSeriesReader will be enhanced. It should clearly state what can be read correctly and what known limitations MITK has
• We accept the implemented handling of this kind of Secondary Capture images as a known issue. Secondary Capture is uncommon for image assessment anyway
• I'll fix the test case as described above
  • a CT and MR copy of the image with all necessary tags
  • keep the current SC copy, add a test that is expected to fail
  • keep another copy of the SC image with an added (0018,2010) tag

Yet another issue: Acquisition Time is required by GdcmSortFunction (crashes otherwise). This tag is optional in DICOM, should not be required.

[abddef]: Merge branch 'bug-7210-DICOMlocaleReopened'

Chose debug output from b

Merged commits:

2011-04-05 15:49:25 Daniel Maleike [837ac9]
Test existance of optional tag "Acquisition time" before reading (crash otherwise)

2011-04-05 15:48:57 Daniel Maleike [542d52]
Use renamed spacing-ok-sc.dcm instead of spacing-ok.dcm

2011-04-05 15:11:38 Daniel Maleike [1afdee]
Start enhancing documentation of DicomSeriesReader

2011-04-05 13:27:37 Daniel Maleike [b67731]
Build CreateMoreUniqueSeriesIdentifier more robust (continue after single tags are missing)

2011-04-05 13:26:57 Daniel Maleike [890297]
Add output of DicomSeriesReader configuration to test case; more helpful output

2011-04-05 13:25:07 Daniel Maleike [6aa0f3]
Fix comment

Merged fixes and minor documentation improvements.

Will add CT/MR test images before closing this ticket.

[82ea4b]: Merge branch 'bug-7210-DICOMlocaleReopened'

Conflicts:
Core/Code/Tes

Merged commits:

2011-04-05 16:35:42 Daniel Maleike [17fe2e]
add tests for CT and MR (GDCM then checks different spacing tags for different storage classes)

2011-04-05 16:28:55 Daniel Maleike [894c35]
Modify y spacing to test correct x,y order of read spacings.

Made cheap copies of the SC image and labelled them as CT/MR. Removed other spacing tags than "(0028,0030) Pixel Spacing" from these images.

Integrated into master.

Adapted Wiki specification, removed core modification flag.

Sascha just commented on T3700:

• MITK DASHBOARD BLOCKER !!! ****

The DICOM locale tests using the mitkDicomSeriesReader.cpp:DicomSeriesReader
class fail on Windows.

In DicomSeriesReader::CreateSeriesIdentifierPart at line 410:

result = IDifyTagValue( tagValueMap[ tag ] );

triggers an assertion failure. tagValueMap returns a zero const char pointer,
but IDifyTagValue expects a std::string and calling std::string(0) triggers the
assertion failure.

Please fix as soon as possible.

(In reply to comment #14)

In DicomSeriesReader::CreateSeriesIdentifierPart at line 410:

401

[9f90ef]: Merge branch 'bug-7210-DICOMlocaleReopened'

Merged commits:

2011-04-08 17:36:17 Daniel Maleike [e2f603]
check for NULL const char* entries in tagValueMap (std::string complains with Visual Studio)

Ok, I just fixed the issue by checking for missing entries in the map and explicitly creating an empty string in that case. I'll pay attention to the continous client.

Testing bugs are part of their respective modules -> changing component to other, please assign appropriate component.