resolved T19783 (implement DICOM RTPlanReader)
6730f2bc195e
Actions

Description

resolved T19783 (implement DICOM RTPlanReader)

Signed-off-by: Clemens Hentschke <c.hentschke@dkfz-heidelberg.de>

Details

Auditors

Provenance

hentsch	Authored on Nov 29 2016, 10:53 AM
hentsch	Pushed on Nov 29 2016, 10:53 AM

Parents

rMITK54c4495d7593: Merge branch 'T20151-MatchPoint_install_rules' Closed T20151

Branches

Unknown

Tags

Unknown

Tasks

T19783: Implement RTPlan reader

Event Timeline

hentsch committed rMITK6730f2bc195e: resolved T19783 (implement DICOM RTPlanReader) (authored by hentsch).Nov 29 2016, 10:53 AM

Hi Ralf, könntest du mal drüberschauen, ob die RTPlanReader Implementierung so ok ist?

One general comment.

In the code that registeres the RT reader services i would also get the DICOMTagOfInterest service and register all tags od RTplan. This will amongst others lead to proper settings for the property descriptions so you have nice namings for the plan tags e.g. in the DICOM property view.

See the Activator of DICOMReaderService as an example. Does the same for the general DICOM tags.

void DICOMReaderServicesActivator::Load(us::ModuleContext* context)
{
  [...]

  DICOMTagPathMapType tagmap = GetDefaultDICOMTagsOfInterest();
  for (auto tag : tagmap)
  {
    m_DICOMTagsOfInterestService->AddTagOfInterest(tag.first);
  }
}

By the way. Nice usage of std and c++11 :)

/Modules/DicomRT/include/mitkRTPlanReader.h
53	The method is not dependent on the state of the class. Either make it static or move it outside of the class as a function that can be called from everyone to get the tags of interest for RTPlans.
53	I would typedef it for the class to get the signitures more readable.
55	I would typedef template-argument-monster to get the signitures more readable.
/Modules/DicomRT/src/mitkRTPlanReader.cpp
90	Not good. You pack together redundant information! mitk::DICOMTagPathToPropertyName(tagPath) is just a deterministc transformation of tagPath. Can be done everywhere lazzy. Makes no sense to do it here, because you gain nothing but introduce design assumpotions and therefore dependencies. You don't want the name generated be tagPath!!! What you want is the path generated in the findings and make a name out of it.
94	Don't think the type encoding. It introduces "magic numbers". When you encode types, why don't you use typeid/typeof... ? I would realy thinking of skipping the type thing totaly. and just store the values. If they are needed converted the using code could do it as well (because it is all standardized by DICOM one knows what he expects). Currently dcm tag values from other readers are also just stored as plain strings. If we want to build in conversion into "closest" values representation we should do it once generally but not distributed conversion strategies all over the code.
116	So at the end of the day I think, in our current state, the function should just return the list of tag paths. Then we also don't need ExtractDicomPathList()
128–139	Some whate obsolete in this form if we look at the discussion below. If we skip the conversion, we only need the findings itself. The rest is not needed.
148	You should not generate the property name like this! Regard my last bug squashing seminar. Use mitk::DICOMTagPathToPropertyName with the tag path given in the finding instance. Then everything is nice, shiny and unique.
149–160	As seed before i would skip the conversion here. Just use a TemperoSpatialStringProperty and store the string value.
/Modules/DicomRT/test/mitkRTPlanReaderTest.cpp
49–64	Must then be actualized to reflect the changes in the code. Especially the changed types and the correct property naming. It will be e.g. DICOM.300A.0010.[0].300A.0016 instead of DICOM.300A.0010.[*].300A.0016.0, because it is a tag path to the first sequence item of 300A,0010.