ENH: Initial RTIonPlan and multiple control points support #124

MichaelColonel · 2019-12-30T19:02:46Z

Greetings,

vtkSlicerDicomRtReader class - RTIonPlan support and control points support, some C-like arrays replaced with std::array for easier copy and assign operations. Scan spot position map inital support for modulated scan type of rt ion beam. Fraction module from DICOM is used to check if beams are available.

vtkSlicerDicomRtImportExportModuleLogic class - each control point is represented as a separate beam. Table nodes are set a children of a corresponding beam nodes.

vtkMRMLRTIonBeamNode class is added for ion beam parameters and visualization.

vtkMRMLRTBeamNode multiple visible sections of MLC opening have been added (corvus-6.2.2-phantom example data control point 10 & 11)

I can split this pull request on several parts if its very big.

cpinter

Happy new year :) Thanks again for the great work! I left a few comments.

Please also

Squash these 47 commits into a single one (or 2-3 commits if they can be cleanly separated) and add descriptive comments in it, explaining in detail what changed and why (for example a line about each original commit, since you made them separately on purpose)
Run the automated tests and make sure no new failures happen

Thank you!

cpinter · 2020-01-06T10:52:16Z

Beams/MRML/vtkMRMLRTBeamNode.cxx

+namespace
+{
+
+typedef std::vector< std::pair< double, double > > PointVector;


Please rename variable to something more meaningful (points of what?)

cpinter · 2020-01-06T10:52:39Z

Beams/MRML/vtkMRMLRTBeamNode.cxx

+{
+
+typedef std::vector< std::pair< double, double > > PointVector;
+typedef std::vector< std::array< double, 4 > > LeafDataVector;


Same here, please don't use the word data, but call it something more informative.

cpinter · 2020-01-06T10:53:33Z

Beams/MRML/vtkMRMLRTBeamNode.cxx

+{
+
+typedef std::vector< std::pair< double, double > > PointVector;
+typedef std::vector< std::array< double, 4 > > LeafDataVector;


Same here, please don't use the word data, but call it something more informative.

cpinter · 2020-01-06T11:05:54Z

Beams/MRML/vtkMRMLRTBeamNode.cxx

@@ -57,6 +58,155 @@ static const char* MLCPOSITION_REFERENCE_ROLE = "MLCPositionRef";
 static const char* DRR_REFERENCE_ROLE = "DRRRef";
 static const char* CONTOUR_BEV_REFERENCE_ROLE = "contourBEVRef";

+//------------------------------------------------------------------------------
+namespace


Why do you use a namespace here? It would be much better making this part of the class

By means of vtkInternal, or just private?

If there is no internal class yet, then no need to bother, just private. Otherwise internal please.

cpinter · 2020-01-06T11:10:20Z

Beams/MRML/vtkMRMLRTIonBeamNode.cxx

+}
+
+//----------------------------------------------------------------------------
+void vtkMRMLRTIonBeamNode::WriteXML(ostream& of, int nIndent)


Please use the new macros defined here
https://github.com/Slicer/Slicer/blob/master/Libs/MRML/Core/vtkMRMLNodePropertyMacros.h
See examples here
https://github.com/Slicer/Slicer/blob/master/Libs/MRML/Core/vtkMRMLDisplayNode.cxx#L110
Also for ReadXMLAttributes, Copy, and PrintSelf

I fixed that, not so sure about Copy method though.

cpinter · 2020-01-06T11:10:56Z

Beams/MRML/vtkMRMLRTIonBeamNode.cxx

+  this->VSADx = xComponent;
+  this->VSADy = yComponent;
+  this->Modified();
+  this->InvokeCustomModifiedEvent(vtkMRMLRTBeamNode::BeamGeometryModified);


In some places you use Superclass, in others the class name itself. Please make it consistent.

It will be Superclass only for overridden methods and inherited constructor.

cpinter · 2020-01-06T11:14:58Z

DicomRtImportExport/Logic/vtkSlicerDicomRtReader.cxx

-      Distance(-1.),
-      NumberOfLeafJawPairs(0)
+      SourceIsoDistance(400.),
+      NumberOfPairs(0)


Why did you rename this variable to have a less informative name? I'm actually curious, so if it's justifiable then no need to change, but in general we prefer longer but unambiguoug names. Not every reader of the code is an expert in every aspect of RT.

I will return previous names.

Thanks! You also changed some names to be more informative, so those should be kept (like the SourceIsoDistance)

SourceIsoDistance parameter has different physical meaning for RTPlan and RTIonPlan but the same DICOM tag (300A,00BA)

For RTPlan its "Source to Beam Limiting Device Distance";

For RTIonPlan its "Isocenter to Beam Limiting Device Distance".

In any case the name of the parameters isn't obvious, but i will keep it.

cpinter · 2020-01-06T11:15:44Z

DicomRtImportExport/Logic/vtkSlicerDicomRtReader.cxx

+  };
+
+  /// Structure storing a treatment compensator parameters associated with beam
+  class CompensatorEntry


Is this class and BlockEntry future work? Since they don't contain anything... In that case please add "//TODO: ..." comments

I will do this.

cpinter · 2020-01-06T11:16:57Z

DicomRtImportExport/Logic/vtkSlicerDicomRtReader.cxx

@@ -103,7 +138,7 @@ class vtkSlicerDicomRtReader::vtkInternal
    unsigned int Number;
    std::string Name;
    std::string Description;
-    double DisplayColor[3];
+    std::array< double, 3 > DisplayColor;


I believe we used a static array because of VTK set/get macros. Is there a good reason to change this?

There is only one method which use DisplayColor
double* vtkSlicerDicomRtReader::GetRoiDisplayColor(unsigned int internalIndex).
The return value is a pointer, so no difference. Everything else is a DCMTK specific.

No setters to change a display color, within internal elements of the reader.

I that case i had thought it's better to use std::array, rather C array (no copy, assign problems).

I can roll it back, if its required to use C array.

If it's only internal then it's fine like this, thanks!

cpinter · 2020-01-06T11:18:08Z

DicomRtImportExport/Logic/vtkSlicerDicomRtReader.cxx

@@ -182,7 +322,7 @@ class vtkSlicerDicomRtReader::vtkInternal
  public:
    ChannelEntry()
    {
-      Number = -1;
+      Number = 0;


Please explain why you changed this. -1 is an obvious "uninitialized" state, while 0 is valid, so this changes the way a newly instantiated object can be handled.

MichaelColonel · 2020-01-07T10:46:18Z

Squashed to 6 commits.

cpinter · 2020-01-07T15:03:07Z

Thank you very much! I'll take a look shortly and integrate.
Please let me know about the automated tests.

MichaelColonel · 2020-01-07T19:10:37Z

23 failed out of 70

 12 - py_DicomRtImportTest (Failed)
 18 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseProstate_Base (Failed)
 19 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseProstate_CERR (Failed)
 20 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseEnt_CERR (Failed)
 21 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseProstate_Eclipse (Failed)
 22 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseEnt_Eclipse (Failed)
 23 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseProstate_CERR_AutomaticOversampling (Failed)
 24 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseEnt_CERR_AutomaticOversampling (Failed)
 25 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseProstate_Eclipse_AutomaticOversampling (Failed)
 26 - vtkSlicerDoseVolumeHistogramModuleLogicTest_EclipseEnt_Eclipse_AutomaticOversampling (Failed)
 27 - vtkSlicerDoseVolumeHistogramModuleLogicTest_DoseSurfaceHistogram_EclipseEnt_Base_Inside (Failed)
 28 - vtkSlicerDoseVolumeHistogramModuleLogicTest_DoseSurfaceHistogram_EclipseEnt_Base_Outside (Failed)
 29 - vtkSlicerDoseVolumeHistogramModuleLogicTest_DoseSurfaceHistogram_EclipseProstate_Base_Inside (Failed)
 30 - vtkSlicerDoseVolumeHistogramModuleLogicTest_DoseSurfaceHistogram_EclipseProstate_Base_Outside (Failed)
 37 - vtkSlicerSegmentMorphologyModuleLogicTest_EclipseProstate_Expand (Failed)
 38 - vtkSlicerSegmentMorphologyModuleLogicTest_EclipseProstate_Shrink (Failed)
 39 - vtkSlicerSegmentMorphologyModuleLogicTest_EclipseProstate_Union (Failed)
 40 - vtkSlicerSegmentMorphologyModuleLogicTest_EclipseProstate_Intersect (Failed)
 41 - vtkSlicerSegmentMorphologyModuleLogicTest_EclipseProstate_Subtract (Failed)
 42 - vtkSlicerSegmentMorphologyModuleLogicTest_EclipseProstate_Intersect_ApplyTransform (Failed)
 45 - vtkSlicerSegmentComparisonModuleLogicTest_EclipseProstate_Base (Failed)
 46 - vtkSlicerSegmentComparisonModuleLogicTest_EclipseProstate_SameInput (Failed)
 47 - vtkSlicerSegmentComparisonModuleLogicTest_EclipseProstate_Transformed (Failed)

MichaelColonel · 2020-01-08T05:57:11Z

Offtopic question. If i have been needed a MLC visualization like in XiO, should i create a custom node for MLC model, or i could use a beam node?

From this post

cpinter · 2020-01-09T11:48:28Z

23 failed out of 70

Well, those will need to be fixed. I see that these failures are the same as what SlicerRT has had since the end of October. It would be great if the tests could be fixed by the developer team so that you can (after a rebase) see if your changes have broken anything or not.
I added a ticket for this (#125) and contacted the team about it. Please stand by for a bit until we see if you can rebase and try it again soon.

cpinter · 2020-01-09T11:50:33Z

About the XiO question: Is this visualization in the 3D view? If so, then special overlay would be needed to achive the same, which will require substantial work. However, you may not need it in the 3D view, in which case it is a simple slice intersection, which could already work with your changes.
Thoughts @gregsharp @lassoan ?

MichaelColonel · 2020-01-09T12:28:27Z

The picture (part of it) is taken from SlicerRtData.

lassoan · 2020-01-09T15:51:52Z

About the XiO question: Is this visualization in the 3D view? If so, then special overlay would be needed to achive the same, which will require substantial work. However, you may not need it in the 3D view, in which case it is a simple slice intersection, which could already work with your changes.
Thoughts @gregsharp @lassoan ?

Since it is a beam's eye view, the camera position, orientation, and viewing angle has match the beam parameters and cannot be rotated. Since the camera cannot be rotated and viewing angle cannot be changed, it does not matter if you display the grid as a 2D or 3D actor. In 3D views it is very easy to display 3D (using model nodes), so I would just go with that. If you create a model that contains projection of the collimator leaves (their shadow) then you can use the exact same model in 2D views (as slice intersection) and in 3D view. Another advantage of this approach is that if for some reason the camera gets rotated or any way misaligned with the beam's eye view, it will still be correct (but you'll see 3D beams and not just a 2D grid). In beam's eye 3D views you can choose to disable camera rotation similarly to how it is done for slice views here.

cpinter · 2020-01-13T16:12:17Z

@MichaelColonel The problem that made most of the tests fail has been fixed by @Sunderlandkyl, and the dashboard shows only one failing test. So I think if you can build this branch against the latest Slicer and only the same tests fails (py_DicomRtImportTest, which tests importing/loading DICOM but not ion plan obviously - it may be added to it though), then I'll integrate this. Thanks again!

MichaelColonel · 2020-01-13T18:13:07Z

All tests have passed except py_DicomRtImportTest. It failed because number of loaded beam nodes, and total number of nodes in hierarchy had increased (each control point is a separated beam node).

After some minor changes:

self.assertEqual( len( slicer.util.getNodes('vtkMRMLModelHierarchyNode*') ), 11 )
self.assertEqual( shNode.GetNumberOfItems(), 42 )

it also passed.

These changes are not in pull request.

cpinter · 2020-01-14T09:24:45Z

@MichaelColonel you are great :) Now that the tests pass, I'll integrate this change, but not before you give me the green light, because I have seen pushes recently, and not sure if you want to add more improvements.

I'll make the changes in py_DicomRtImportTest and close the related ticket. Thanks again!

MichaelColonel · 2020-01-15T04:05:38Z

The pull request is ready to be merged, latest pushes were minor API style changes - GetBeamControlPoint* instead of GetControlPoint*.

cpinter requested changes Jan 6, 2020

View reviewed changes

MichaelColonel force-pushed the ion-plan-support branch 2 times, most recently from bb48371 to 67465ed Compare January 7, 2020 10:09

Michael Colonel and others added 4 commits January 7, 2020 13:12

ENH: RTIonPlan and control points support

46b630e

ENH: vtkMRMLRTIonBeamNode added

2c432a2

ENH: RTIonPlan and control points cleanups and fixes

24ad21d

ENH: beams are parents for observed table nodes in hierarchy

c279962

MichaelColonel force-pushed the ion-plan-support branch from 67465ed to 66696ec Compare January 7, 2020 10:14

FIX: MLC distance and JawPositions proper default values

89834f5

MichaelColonel pushed a commit to MichaelColonel/SlicerRT that referenced this pull request Jan 7, 2020

FIX: Pull request SlicerRt#124 fixes

67f943b

MichaelColonel force-pushed the ion-plan-support branch from 66696ec to 67f943b Compare January 7, 2020 10:23

MichaelColonel pushed a commit to MichaelColonel/SlicerRT that referenced this pull request Jan 7, 2020

ENH: Pull request SlicerRt#124 cleanups and fixes

2faa5b4

MichaelColonel force-pushed the ion-plan-support branch from 67f943b to 2faa5b4 Compare January 7, 2020 18:32

cpinter mentioned this pull request Jan 12, 2020

BUG: Fix vtkITKArchetypeImageSeriesReader::CanReadFile() Slicer/SlicerGitSVNArchive#1269

Closed

MichaelColonel pushed a commit to MichaelColonel/SlicerRT that referenced this pull request Jan 13, 2020

ENH: Pull request SlicerRt#124 fixes

70da8e0

MichaelColonel pushed a commit to MichaelColonel/SlicerRT that referenced this pull request Jan 13, 2020

ENH: Pull request SlicerRt#124 cleanups and fixes

64598df

MichaelColonel force-pushed the ion-plan-support branch from 70da8e0 to 64598df Compare January 13, 2020 07:47

MichaelColonel pushed a commit to MichaelColonel/SlicerRT that referenced this pull request Jan 13, 2020

ENH: Pull request SlicerRt#124 cleanups and fixes

95dd9f2

MichaelColonel force-pushed the ion-plan-support branch from 64598df to 95dd9f2 Compare January 13, 2020 07:59

MichaelColonel added a commit to MichaelColonel/SlicerRT that referenced this pull request Jan 13, 2020

ENH: Pull request SlicerRt#124 fixes

c778013

MichaelColonel pushed a commit to MichaelColonel/SlicerRT that referenced this pull request Jan 13, 2020

ENH: Pull request SlicerRt#124 fixes 2

c415a5d

ENH: Pull request SlicerRt#124 cleanups and fixes

908a94b

MichaelColonel force-pushed the ion-plan-support branch from c415a5d to 908a94b Compare January 13, 2020 17:10

cpinter mentioned this pull request Jan 15, 2020

py_DicomRtImportTest failing #126

Closed

cpinter merged commit 93f0df9 into SlicerRt:master Jan 15, 2020

cpinter mentioned this pull request Jan 15, 2020

Build error after integrating ion plan support #129

Closed

MichaelColonel mentioned this pull request Jan 15, 2020

Subject hierarchy problems after beam changes #128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Initial RTIonPlan and multiple control points support #124

ENH: Initial RTIonPlan and multiple control points support #124

MichaelColonel commented Dec 30, 2019

cpinter left a comment

cpinter Jan 6, 2020

cpinter Jan 6, 2020

cpinter Jan 6, 2020

cpinter Jan 6, 2020

MichaelColonel Jan 6, 2020

cpinter Jan 7, 2020

cpinter Jan 6, 2020

MichaelColonel Jan 7, 2020

cpinter Jan 6, 2020

MichaelColonel Jan 6, 2020 •

edited

Loading

cpinter Jan 6, 2020

MichaelColonel Jan 6, 2020

cpinter Jan 7, 2020

MichaelColonel Jan 7, 2020

cpinter Jan 6, 2020

MichaelColonel Jan 6, 2020

cpinter Jan 6, 2020

MichaelColonel Jan 6, 2020

cpinter Jan 7, 2020

cpinter Jan 6, 2020

MichaelColonel commented Jan 7, 2020

cpinter commented Jan 7, 2020

MichaelColonel commented Jan 7, 2020

MichaelColonel commented Jan 8, 2020 •

edited

Loading

cpinter commented Jan 9, 2020 •

edited

Loading

cpinter commented Jan 9, 2020

MichaelColonel commented Jan 9, 2020

lassoan commented Jan 9, 2020

cpinter commented Jan 13, 2020

MichaelColonel commented Jan 13, 2020 •

edited

Loading

cpinter commented Jan 14, 2020

MichaelColonel commented Jan 15, 2020

ENH: Initial RTIonPlan and multiple control points support #124

ENH: Initial RTIonPlan and multiple control points support #124

Conversation

MichaelColonel commented Dec 30, 2019

cpinter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelColonel Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelColonel commented Jan 7, 2020

cpinter commented Jan 7, 2020

MichaelColonel commented Jan 7, 2020

MichaelColonel commented Jan 8, 2020 • edited Loading

cpinter commented Jan 9, 2020 • edited Loading

cpinter commented Jan 9, 2020

MichaelColonel commented Jan 9, 2020

lassoan commented Jan 9, 2020

cpinter commented Jan 13, 2020

MichaelColonel commented Jan 13, 2020 • edited Loading

cpinter commented Jan 14, 2020

MichaelColonel commented Jan 15, 2020

MichaelColonel Jan 6, 2020 •

edited

Loading

MichaelColonel commented Jan 8, 2020 •

edited

Loading

cpinter commented Jan 9, 2020 •

edited

Loading

MichaelColonel commented Jan 13, 2020 •

edited

Loading