Page MenuHomePhabricator

Refactoring of DicomSeriesReader
Closed, ResolvedPublic

Description

In its current version, class DicomSeriesReader has around 2500 lines of code, which makes it hard to maintain.
In addition, there have been cases where the hard-coded default frame sorting algorithm fails to work in an expected way (e.g. multi-phase acquisitions with minimal shift between phases).

I have been working on a new version of the reader which is now ready for a review and could soon be integrated into master. The code

  • is structured accross multiple classes in a module of its own
  • is more flexible / configurable regarding the frame sorting
  • can be more easily enhanced to cover multi-frame DICOM images
  • will be helpful for the diffusion module (disussed with Jan Hering)

The current tests from module DICOMTesting were moved to test the new infrastructure and are by now passing again.

Summary of the structure of the work-in-progress branch:

  • new module DICOMReader
  • module DICOMTesting moved from Core to Modules
    • and changed to use/test the new code
  • Doxygen documentation is in this branch is considered part of this bug's documentation. When generated, the most important parts are
    • group__DICOMReaderModule.html
    • classmitk_1_1DICOMITKSeriesGDCMReader.html
    • regarding flexibility:
      • classmitk_1_1DICOMFileReaderSelector.html
      • classmitk_1_1DICOMReaderConfigurator.html

To visually test the code within mitkWorkbench, I also pushed a debugging branch
personal/maleike/dcm-ugly-loader
which reuses the file import button of the DICOM editor to load files via the new code. (this branch should never be integrated)

A number of smaller issues is marked with TODO lines in the code itself.

We can setup a meeting if this helps in discussing this branch.

Event Timeline

maleike added a subscriber: maleike.

New remote branch pushed: bug-16744-dcm-refactor

Work on this topic has now been finished. Markus Engel will prepare a review of the code. Two things to improve before integration:

  • extensive testing of the new flexible sorting
    • some of that is tested via module DICOMTesting
  • performance improvements of the DICOMFileReaderSelector class, which currently scans the same DICOM files multiple times

So far, I have checked the new classes for general code-related issues:

DICOMITKSeriesGDCMReader:

  • Copy c'tor and operator= do not copy all attributes
    • m_SortingResultInProgress
    • m_ReplacedCLocales
    • m_ReplacedCinLocales
    • m_InputFrameList
    • m_GDCMScanner

DICOMFileReader:

  • Copy c'tor does not copy all attributes
    • m_InputFilenames

DICOMTagBasedSorter

  • Copy c'tor and operator= do not copy all attributes
    • m_DistinguishingTags
    • m_TagValueProcessor
    • m_SortCriterion
  • operator== does not consider attributes
    • m_DistinguishingTags
    • m_TagValueProcessor
    • m_SortCriterion

Checking the actual functionality of the new readers will be done in the next couple of days by massively importing images...

(In reply to Markus Engel from comment #3)

So far, I have checked the new classes for general code-related issues:

Thanks Markus, I've fixed these issues with the most recent commits

(In reply to Daniel Maleike from comment #2)

of the code. Two things to improve before integration:

  • performance improvements of the DICOMFileReaderSelector class, which currently scans the same DICOM files multiple times

Performance is now greatly improved by factoring out DICOMGDCMTagScanner. Now each file is read only once (instead of 5 or 6 times).

Next thing: I'll check master integration, which seems a bit more complicated. Then I'll be working more on tests with realistic and problematic data.

New remote branch pushed: bug-16744-old-branch-base

New remote branch pushed: bug-16744-integration-snapshot-201308

New remote branch pushed: bug-16744-integration-master

For integration into MITK master, I reconstructed the branch, so now i have three branches pushed:

bug-16744-old-branch-base :

head of current development, can be further developed and merged into
the following two branches when features evolve

bug-16744-integration-snapshot-201308

integrates bug-16744-old-branch-base with releases/snapshot-201308

bug-16744-integration-master

integrates bug-16744-old-branch-base with master

For master integration, I had to some CMakeLists.txt, mostly in the modules that I touched.

Also, I discovered a small but interesting difference between the unit tests for the old and new reader classes (in bug-16744-integration-master only): some tests, e.g. DICOM_Load_basic (new code) vs. DCM_Load_basic (old code)
produce image spacings with tiny differences. I'll have to check where this comes from. I suspected the ScalarType float/double conversion, but this should have equal effect on old and new code..

New remote branch pushed: bug-16744-develop

Markus, do you know something about the status here?

Well, as far as I know, the refactoring is finished. Daniel has invested a massive amount of time in testing and bugfixing and everything seems to work real fine right now.

Actually I was not aware that his changes have not yet been integrated into master.

However, I am not sure about integrating the changes now, just before the release, as it will need extensive testing on your side.

Furthermore, I do not know which state the branches are in. I know that Daniel had seperate branches for integration into master and our snapshot, but I am not sure if he has always correctly commited all fixes into both branches..

so bug-16744-develop is the latest branch, or is there a newer one ?

Current release is finished. Resetting target milestone

User hering has pushed new remote branch:

bug-16744-dcm-refactor-integration-2014-03

Since there is no wiki specification page available I had a quick look at the changes regarding the core:

  • folder DICOMTestion was renamed to DCMTesting
  • mitkLevelWindow and ImageVtkMapper2D differ in one line which I will discuss with the rendering guys before merging
  • The rest of the core changes concern just testing or documentation

User fetzer has pushed new remote branch:

bug-16744-dcm-refactor-final-master-integration

[140857]: Merge branch 'bug-16744-dcm-refactor-final-master-integration'

Merged commits:

2014-08-13 15:54:35 Jan Hering [dc6b30]
Fixed 3DnT DICOM Testing

  • checking against the time bound of the whole image
  • adapted MITK-Data Hashtag with correct reference values

2014-08-13 14:04:07 Andreas Fetzer [7e4f34]
Adaptions to most recent MITK Core changes


2014-06-26 15:06:18 Andreas Fetzer [db5e70]
Merge branch 'bug-16744-dcm-refactor-integration-2014-03' into bug-16744-dcm-refactor-final-master-integration

Conflicts:
Core/Code/Testing/DCMTesting/mitkTestDCMLoading.cpp
Core/Code/Testing/DCMTesting/mitkTestDCMLoading.h


2014-06-26 14:16:34 Andreas Fetzer [62493e]
Adding removed tests since the classic is reader is still in use


2014-04-23 15:25:31 Jan Hering [f56a31]
Merge remote-tracking branch 'origin/bug-16744-develop' into bug-16744-dcm-refactor-integration-2014-03

Conflicts:
Core/Code/DataManagement/mitkPropertyList.cpp


2014-04-23 15:15:08 Jan Hering [23d72d]
Adapted DICOM classes to new module naming


2014-04-23 14:36:34 Jan Hering [4d8006]
Merge remote-tracking branch 'origin/bug-16744-integration-master' into bug-16744-dcm-refactor-integration-2014-03

Conflicts:
Core/Code/Rendering/mitkImageVtkMapper2D.cpp
Core/Code/Testing/DCMTesting/mitkTestDCMLoading.h
Core/Code/Testing/DICOMTesting/CMakeLists.txt
Examples/Plugins/org.mitk.example.gui.opencv/CMakeLists.txt
Modules/CMakeLists.txt


2014-02-25 16:35:03 Daniel Maleike [be0c62]
Handle exceptions during string trimming


2014-02-13 12:50:40 Daniel Maleike [7b7df1]
Fix parsing of spacing (was completely broken, see failing tests)


2014-02-13 10:53:28 Daniel Maleike [b704eb]
Improve configuration descriptions: more details


2014-02-13 10:47:10 Daniel Maleike [5e95a4]
Reword a "strange special case" to "ExpectDistanceOne" and change its implementation

This commit adds a flag m_ExpectDistanceOne to DICOMTagBasedSorter, which handles
a special case during "strict sorting". Documentation was enhanced to describe
this.

The default for "strict sorting" has been changed, the new default is
"no strict sorting". XMLs have been changed, so that we use strict sorting
for Instance Number and Slice Location now.


2014-02-12 11:25:31 Daniel Maleike [b3d52e]
Replace atof by istringstream


2014-02-06 15:18:04 Daniel Maleike [ebdc11]
Fix memory leak


2014-02-05 18:04:43 Daniel Maleike [97bc41]
Remove debug message


2014-02-05 18:00:12 Daniel Maleike [1af8c2]
Convert only first component of WindowCenter/WindowWidth to number!


2014-02-05 15:01:37 Daniel Maleike [e55891]
Add a repeated loading step to mitkdump


2014-02-05 15:08:33 Daniel Maleike [16b17b]
Add a "soft" sorting by Instance Number


2014-02-05 15:01:51 Daniel Maleike [3f27cc]
Make "strict sorting" really strict


2014-02-03 17:00:45 Markus Engel [6f00a8]
added additional properties to mitk::Image


2014-01-29 15:10:07 Daniel Maleike [c8eb8c]
Merge remote-tracking branch 'origin/bug-16744-develop' into bug-16744-integration-master


2014-01-29 15:06:19 Daniel Maleike [ad4e53]
Do not return classic reader, seems never the best choice


2014-01-24 16:54:50 Daniel Maleike [cb1efb]
Merge branch 'bug-16744-develop' into bug-16744-integration-master


2014-01-24 16:52:50 Daniel Maleike [e81135]
Fix issues with threaded loading (remove statics)


2014-01-24 16:52:28 Daniel Maleike [20d184]
Fix reference to temporary


2014-01-23 17:03:56 Daniel Maleike [36e816]
Merge branch 'bug-16744-develop' into bug-16744-integration-master


2014-01-23 17:00:41 Daniel Maleike [f9424c]
Fix build system to MITK master


2014-01-23 17:00:27 Daniel Maleike [8a0a22]
Relax test code as in MITK master


2014-01-22 17:39:07 Daniel Maleike [f7d978]
Adapt CMake code to recent master build system changes


2014-01-22 18:05:48 Daniel Maleike [699469]
Add include guards


2014-01-09 14:06:02 Daniel Maleike [5ff709]
Adapting microservices calls to v2

Conflicts:

Modules/DICOMReader/mitkDICOMFileReaderSelector.cpp


2014-01-22 18:07:14 Daniel Maleike [855e4a]
Resolve forgotten merge conflict


2014-01-22 16:46:37 Daniel Maleike [9f4cef]
Merge remote-tracking branch 'origin/master' into bug-16744-integration-master

Conflicts:
Core/Code/Common/mitkTestingMacros.h
Core/Code/Testing/CMakeLists.txt
Core/Code/Testing/DCMTesting/mitkTestDCMLoading.cpp
Core/Code/Testing/mitkDicomSeriesReaderTest.cpp
Examples/CMakeLists.txt
Modules/CMakeLists.txt
Modules/DICOMTesting/CMakeLists.txt
Modules/ImageStatistics/Testing/mitkImageStatisticsCalculatorTest.cpp


2014-01-22 13:43:05 Daniel Maleike [0822f8]
Fix copy c'tors and operator= (Markus' review)


2014-01-22 11:53:09 Daniel Maleike [5f3bf5]
Clean origin of required tags, check on assumed "tilt bug"


2014-01-22 11:21:05 Daniel Maleike [f72f48]
Nicer timing output


2014-01-22 10:55:07 Daniel Maleike [e6795f]
Make DICOMGDCMTagScanner::GetTagValue() more robust, document class and outstanding issue


2014-01-22 10:54:22 Daniel Maleike [630443]
Update obsolete TODOs and includes


2014-01-21 17:12:47 Daniel Maleike [19be44]
Prepare external tags scanning


2014-01-21 16:52:39 Daniel Maleike [42cff3]
Move some code into GetTagsOfInterst for later reuse


2014-01-21 16:46:06 Daniel Maleike [94908d]
Move actual tag scanning out of DICOMITKSeriesGDCMReader


2014-01-21 15:21:58 Daniel Maleike [067126]
Make tilt and 3D+t properties of ClassicReader configurable


2014-01-21 14:29:08 Daniel Maleike [7e0ad7]
Make sure that level/windows for CR images (values around 16000) work


2014-01-16 17:44:49 Daniel Maleike [95f030]
Implement two more options: "strict sorting" and "accept two slices in group"

  • strict sorting will split a group if the distance of the first-level sorting tag is not constant (missing slices)
  • accept two slices means keeping groups of only two images. this in constrast to earlier implementations, which would assume grouping of "only" two images meaningless

2014-01-16 10:57:46 Daniel Maleike [5742e0]
Require version attribute in config files


2014-01-16 10:43:35 Daniel Maleike [025069]
Fix DICOMTagBasedSorter::operator==()


2014-01-14 18:05:22 Daniel Maleike [b0326b]
Add serialization capability to DICOMReaderConfigurator


2014-01-13 15:09:56 Daniel Maleike [645531]
Fix code


2014-01-13 15:07:31 Daniel Maleike [9fc54d]
Make sure an empty NumberOfFrames tag does not keep us from reading a file


2014-01-13 11:37:32 Daniel Maleike [b4587a]
Add method to add whole readers to the selection process


2014-01-13 11:12:26 Daniel Maleike [fb3779]
Export classes used by other modules (or test drivers)


2014-01-13 10:43:27 Daniel Maleike [985d6e]
Restore old DICOMTesting module and tests as DCMTesting module


2014-01-11 16:25:46 Daniel Maleike [8c6ba2]
Minimum optimization


2014-01-11 16:05:15 Daniel Maleike [25735e]
Make precision of image orientation configurable; make CutDecimalPlaces generic


2014-01-11 15:24:03 Daniel Maleike [3dd416]
Provide properties to pre-loaded mitk::Images. Lets the mitkDICOMPreloadedVolumeTests pass again.


2014-01-11 12:51:21 Daniel Maleike [0fba7d]
Let reader check for DICOMness of inputs, reject processing otherwise


2014-01-11 11:58:06 Daniel Maleike [7e6a28]
Add option for tolerated origin error in EquiDistantBlocksSorter

(and use this option to make "classic" reader behave as the actual classic reader


2014-01-11 11:09:21 Daniel Maleike [ba90ed]
Fix gantry tilt loading, which was broken by prior commits

Several problems were introduced earlier:

  • the NormalDirectionConsistencySorter did not care for tilts but assumed normal direction == origin direction
  • the loaders used one identical gantry-tilt info object for all outputs, which does not make sense with multiple (usually different) outputs

2014-01-11 10:54:36 Daniel Maleike [3b3f76]
Don't always split by acquisition number


2014-01-11 10:53:50 Daniel Maleike [c99e83]
Document potential bug


2014-01-10 14:58:56 Daniel Maleike [e54e8e]
Load 3D if we have only one timestep


2014-01-10 14:10:40 Daniel Maleike [c652b3]
Provide method to load mitk::Image from a block descriptor only


2014-01-10 13:35:14 Daniel Maleike [f91b39]
Provide photometric interpretation and level/window from DICOM images


2014-01-10 11:54:16 Daniel Maleike [9938e7]
Fix implementation of DICOMTag::operator<


2014-01-10 11:46:36 Daniel Maleike [9c5e6d]
Return actual readers from selector class


2014-01-10 10:21:42 Daniel Maleike [b2ab2b]
Lazy generation of properties for imageblockdescriptors


2014-01-10 10:20:49 Daniel Maleike [691980]
Use Acquisition Time instead of ContentTime


2014-01-10 10:16:12 Daniel Maleike [ffc808]
Check if the complete string was converted to a number


2014-01-10 10:14:38 Daniel Maleike [60e654]
Trim whitespace after tag values


2014-01-10 10:14:06 Daniel Maleike [9b2f8b]
More information into log file


2014-01-09 16:50:55 Daniel Maleike [44ea34]
Make compile on Windows


2014-01-09 16:44:36 Daniel Maleike [e093d9]
Compile on Windows and fix error


2014-01-09 14:06:12 Daniel Maleike [1fd922]
Updating TODO list


2014-01-09 13:47:07 Daniel Maleike [e18079]
Add option to load images and display information about first/last slice


2014-01-09 13:43:38 Daniel Maleike [e057b6]
Fix upside-down images that appeared in some cases


2014-01-08 18:05:48 Daniel Maleike [47a0a3]
Start documenting TODOs


2014-01-08 18:05:41 Daniel Maleike [cdc851]
Fix typo


2014-01-08 13:53:03 Daniel Maleike [0e72e9]
Add order to xml configs

Conflicts:

Modules/DICOMReader/files.cmake


2014-01-08 13:03:53 Daniel Maleike [e8873a]
Compact output of mitkdump and work directories recursively


2014-01-08 11:06:11 Daniel Maleike [59e2a7]
Improve output of the mitkdump tool


2014-01-07 17:00:47 Daniel Maleike [213417]
Add overview diagram


2014-01-07 16:22:47 Daniel Maleike [60b420]
Documentation for most of the module's classes


2013-12-20 17:32:48 Daniel Maleike [008865]
Starting documentation


2013-12-20 15:19:27 Daniel Maleike [594824]
Add description to configurations


2013-12-20 14:54:50 Daniel Maleike [9529c7]
Formulate more loading options


2013-12-20 14:53:51 Daniel Maleike [ab6d9d]
Improve tolerance regarding origin positions


2013-12-19 18:06:11 Daniel Maleike [7cadab]
Fix syntax (remove commas)


2013-12-19 12:58:48 Daniel Maleike [94e131]
Implement selection of best reader


2013-12-19 12:13:34 Daniel Maleike [da1f99]
Ability to describe configuration; fixing forced sorting elements


2013-12-18 18:27:14 Daniel Maleike [3f4a63]
WIP: Update configurator class to support at least two classes


2013-12-18 15:29:24 Daniel Maleike [84bb30]
Configurator for DICOM series reader (code by Andreas Fetzer)

Code was not cherry-picked, because original commits
did not contain license headers (not pushable without)
and also contained a GUI module (I'd prefer to start without)


2013-12-18 14:52:20 Daniel Maleike [556c92]
Provide mitk properties to describe outputs


2013-12-17 18:12:46 Daniel Maleike [fd5ba9]
WIP: Add basic slice description via properties


2013-12-17 15:24:13 Daniel Maleike [bd3d9d]
Handle locales during AnalyzeImages and LoadImages


2013-12-17 14:57:34 Daniel Maleike [a265cb]
Remove obsolete TODOs

(test is implemented in a different method, copy is actually done)


2013-12-17 14:57:00 Daniel Maleike [8699d9]
FixupSpacing in DICOMImageBlockDescriptor


2013-12-17 14:49:57 Daniel Maleike [755fe3]
Remove obsolete methods


2013-12-17 14:49:48 Daniel Maleike [f62703]
InPlaceFixUpTiltedGeometry -> FixUpTiltedGeometry


2013-12-17 14:44:50 Daniel Maleike [e8c3c5]
Remove some simple TODOs, update others


2013-12-17 14:10:49 Daniel Maleike [521747]
INFO -> DEBUG


2013-12-17 14:08:31 Daniel Maleike [725402]
Put hard requirements into DICOMITKSeriesGDCMReader


2013-12-17 13:45:47 Daniel Maleike [9fe957]
3D+t loading


2013-12-14 00:44:23 Jan Hering [1d8d8c]
Fixed float -> ScalarType in BlockDescriptor


2013-12-13 19:08:30 Daniel Maleike [e140a6]
Updating TODOs a bit


2013-12-13 18:22:54 Daniel Maleike [72db1e]
Fix image position sorting (formerly: wrong indices used)


2013-12-13 18:22:13 Daniel Maleike [9a2f48]
Fix tilt calculation precision


2013-12-12 16:36:42 Daniel Maleike [a3e75b]
Implement ganry tilt "correction" by image shearing


2013-12-12 16:34:09 Daniel Maleike [3876e2]
Use mitk::ScalarType instead of float


2013-12-12 12:54:46 Daniel Maleike [055ab2]
Consider only a configurable number of digits for ImageOrientation


2013-12-12 12:52:59 Daniel Maleike [2b83f0]
More debugging output


2013-12-11 17:26:18 Daniel Maleike [da6cd2]
Prefer Pixel Spacing over Imager Pixel Spacing


2013-12-11 17:25:52 Daniel Maleike [1e16ec]
Handle defaults for Image Position (Patient) and Image Orientation


2013-12-11 16:27:26 Daniel Maleike [763b4a]
Sorter for ImagePositionPatient/ImageOrientationPatient


2013-12-11 15:32:03 Daniel Maleike [9c1f26]
Removing test of questionable value


2013-12-11 15:24:14 Daniel Maleike [63600a]
Start fixing DICOM tests, using ClassicDICOMSeriesReader in ImageStatisticsTest


2013-12-11 15:23:09 Daniel Maleike [6ec4b4]
Use DICOMTag in mitkdump


2013-12-11 15:22:45 Daniel Maleike [f4975a]
Implemented IsDICOM()


2013-12-11 11:31:33 Daniel Maleike [47d57f]
Move DICOMTesting from Core/Testing to Modules/


2013-12-11 11:55:36 Daniel Maleike [392af6]
Add class DICOMTag for brevity


2013-12-11 11:32:04 Daniel Maleike [eea6ab]
ClassicDICOMSeriesReader mimics old DicomSeriesReader with new classes


2013-12-11 00:31:21 Daniel Maleike [0fa147]
slice distance sorter


2013-12-10 17:08:43 Daniel Maleike [cf5e63]
mitkdump commandline tool to predict output


2013-12-10 15:52:58 Daniel Maleike [1f9e44]
Implement basic Image loading


2013-12-09 09:23:42 Daniel Maleike [c4f299]
WIP sorting


2013-12-06 19:00:58 Daniel Maleike [1e1202]
Basic splitting/sorting classes


2013-12-05 17:31:54 Daniel Maleike [3ed22b]
Basic test (all inputs are distributed to outputs)


2013-12-05 17:31:28 Daniel Maleike [83b744]
Include guards for test macros


2013-12-05 15:42:13 Daniel Maleike [4343d6]
Prepare sorting for DICOMITKSeriesGDCMReader


2013-12-04 18:23:35 Daniel Maleike [be4767]
FileReader and BlockDescriptor

[7ef30f]: Merge branch 'bug-16744-dcm-refactor-final-master-integration'

Merged commits:

2014-08-13 16:33:51 Jan Hering [f4d0da]
COMP: Fixed export macro & include

[665164]: Merge branch 'bug-16744-dcm-refactor-final-master-integration'

Merged commits:

2014-08-13 22:10:04 Andreas Fetzer [9a595b]
COMP Fixed float handling for windows

New DICOM Reader was merged. The actual integration/usage and replacement of the old on will be adressed in separate bugs