From IGSTK

Contents 1 Classes Scheduled for Code Reviews 2 Classes still in development 3 Reviews 3.1 AtamaiNDITracker 3.2 FlockOfBirdsCommandInterpreter 3.2.1 Andinet's comments 3.2.2 Julien's comments 3.3 FlockOfBirdsTracker 3.3.1 Andinet's comments 3.3.2 David's comments 3.4 FlockOfBirdsTrackerTool 3.4.1 Andinet's comments 3.4.2 David's Comments 3.5 MR3DImageToUS3DImageRegistration 3.5.1 David's Comments 3.5.2 Luis' Comments 3.6 VascularNetworkObject 3.6.1 Patrick's Comments 3.6.2 Luis's Comments 3.7 VascularNetworkObjectRepresentation 3.7.1 Patrick's Comments 3.7.2 Luis' Comments 3.8 VascularNetworkReader 3.8.1 Patrick's Comments 3.8.2 Luis's Comments 3.9 VesselObject 3.9.1 Patrick's Comments 3.9.2 Luis' Comments 3.10 USImageReader 3.10.1 Andinet's Comments 3.10.2 Luis' Comments 3.11 USImageObject 3.11.1 Andinet's comments 3.11.2 Luis' Comments 3.12 USImageRepresentation 3.12.1 Andinet's comments 3.12.2 Luis' Comments 3.13 USImageSimulator 3.13.1 Luis' Comments 3.14 ToolCalibration 3.14.1 David's comments 3.14.2 Luis' comments 3.15 ToolCalibrationReader 3.15.1 David's comments 3.15.2 Luis' comments 3.16 PivotCalibrationReader 3.16.1 David's comments 3.16.2 Luis' comments

Classes Scheduled for Code Reviews

These classes are ready for immediate review.

Please fix the code when you do the review

Name	Author	Reviewer 1	Reviewed	Code Fixed	Reviewer 2	Reviewed	Code Fixed	Moved
FlockOfBirdsCommandInterpreter	DG	JJ	Y	N	AE	Y	Y	N
FlockOfBirdsTracker	JJ	DG	Y	Y	AE	Y	Y	N
FlockOfBirdsTrackerTool	JJ	DG	Y	Y	AE	Y	Y	N
MR3DImageToUS3DImageRegistration	JJ	DG	Y	Y	LI	Y	N	N
USImageObject	JJ	AE	Y	N	LI	Y	Y	N
USImageObjectRepresentation	JJ	AE	Y	N	LI	Y	Y	N
USImageReader	JJ	AE	Y	N	LI	Y	Y	N
UltrasoundImageSimulator	JJ	AE	Y	N	LI	Y	Y	N
VascularNetworkObject	JJ	PC	Y	N	LI	Y	N	N
VascularNetworkObjectRepresentation	JJ	PC	Y	N	LI	Y	N	N
VascularNetworkReader	JJ	PC	Y	N	LI	Y	N	N
VesselObject	JJ	PC	Y	N	LI	Y	N	N
ToolCalibration	JJ	DG	Y	Y	LI	Y	Y	N
ToolCalibrationReader	JJ	DG	Y	Y	LI	Y	Y	N
PivotCalibrationReader	JJ	DG	Y	Y	LI	Y	Y	N

Classes still in development

• ContourMeshObjectRepresentation
• ContourObjectRepresentation
• ContourVascularNetworkObjectRepresentation
• ObliqueImageSpatialObjectRepresentation
• SocketCommunication (?)
• AtamiNDItracker (deprecate)
• LandmarkUltrasoundCalibration

Reviews

AtamaiNDITracker

• (DG) class has been removed from repository, it was obsoleted by PolarisTracker and AuroraTracker

FlockOfBirdsCommandInterpreter

Andinet's comments

• well written class.. no comments

Julien's comments

• Need more code coverage

FlockOfBirdsTracker

Andinet's comments

-Well written class no comments

David's comments

• Unlike Aurora, the Flock of Birds doesn't have SROMs so SROM code isn't needed (fixed)
• m_PortHandle[] array isn't needed, Flock of Birds does not use port handles for tools (fixed)
• Mistakes in class description (fixed)
• Threading was enabled, but the InternalThreadedUpdateStatus() method was empty so the tracking thread was just spinning its wheels (fixed)

FlockOfBirdsTrackerTool

Andinet's comments

- style issue (fixed)

David's Comments

• simple class, no issues

MR3DImageToUS3DImageRegistration

David's Comments

• header file contains no description of class (description should include information about which metric and optimizer are used) (fixed)
• in CalculateRegistrationProcessing(): (fixed)
  • variable newVersor is declared but not used
  • a few comments would be useful, some of the code here is confusing to me:
    • "finalparams[0] -= initialParameters[0]" - there isn't any explanation for why this subtraction is done, and also I'm not sure if the subtraction of the versor parameters of the transformation makes sense, shouldn't quaternion division be used for versors?

Luis' Comments

• CommandIterationUpdate shouldn't be declared as a public class, at least not with a non-specific name. It has now been moved into a specific namespace. Otherwise, there is the risk of other future registration classes defining objects with clashing names. (fixed)
• Many messages are going to cout instead of the Logger. (fixed)
• Brief Doxygen documentation is missing the terminal "." (fixed).
• Many typedefs that are not part of the public API were moved to the protected section (fixed).
• All the "Processing" methods must be in the private section, not the protected section. (fixed)
• Macros for Observers shouldn't be public in this class. They are part of the internal implementation. (fixed)
• The class takes both CT and MR images, but emphasis is made in MR. (not fixed, requires future discussion)
  • Should we remove CT ?
  • or make them symmetric ?
• BUG: GotUSTransform() was not being checked in the observer before invoking GetUSTransform(). (fixed)

VascularNetworkObject

Patrick's Comments

• no need for including header igstkMacros.h and igstkEvents.h (fixed)
• fix typo in doxygen and comments (fixed)
• No test for this class (fixed)
• Add VesselObject(const Self&) and void operator=(const Self&) method to enforce smart pointer standards (fixed);
• No state machine for this class (fixed)

Luis's Comments

• Full doxygen comment was missing (fixed).
• GetVessel() is an unsafe method. It should be protected by StateMachine logic or replaced with iterators so that it becomes impossible to request an unexisting vessel. (fixed)
• Test is missing. Code coverage = 0% (fixed)

VascularNetworkObjectRepresentation

Patrick's Comments

• no need for including header igstkMacros.h, igstkStateMachine.h, and igstkEvents.h (FIXED)
• fix typo in doxygen and comments (FIXED)
• Why do we create two sphere actors but not adding them to the sence? Is it testing code? (Answer by JJ: The spheres are added to the scene)
  • Around line 160 - 210 in igstkVascularNetworkObjectRepresenration.cxx
  • If this code can be taken out, then no need for including vtkSphereSource.h
• PrintSelf function should add more print self statements (FIXED)
• No test for this class (fixed: Same test as VascularNetworkReader because there is no way to create a vascular network by hand)
• Add VascularNetworkObjectRepresentation(const Self&) and void operator=(const Self&) method to enforce smart pointer standards (fixed);

Luis' Comments

• Doxygen full comment improved (fixed)
• Constructor and destructor don't need "void" argument. (fixed)
• Test is missing. Code coverage = (fixed)
• Printself was missing to print pointer to VascularNetworkObject (fixed)
• What is the notion of "spacing" in a vessel ?
  • is it the spacing of the image from which this vessel was segmented ? (Answer by JJ: yes)
  • Why do the point positions need to be multiplied by spacing ? are they in pixel coordinates ? (Answer by JJ: the actual segmentation is done in pixel coordinates)

VascularNetworkReader

Patrick's Comments

• no need for including header igstkMacros.h and igstkEvents.h (fixed)
• typo in doxygen and comments (fixed)
• No test for this class (fixed)
• No state machine for this class (fixed)

Luis's Comments

• typo in doxygen comment (fixed)
• GetOutput() is an unsafe method. It returns a pointer without indication on whether the pointer is valid. E.g. there is no confirmation that the read was successful. This call should be protected by StateMachine logic. (fixed)
• There is no test for this class. Code coverage = 0% (fixed)
• The Event "VascularNetworkReadingErrorEvent" is never used. State Machine logic is incomplete. (fixed)

VesselObject

Patrick's Comments

• VesselObject naming issue (Do we have an agreement for the spatial object name yet) (answer by JJ: will be fixed later with other classes)
• No need to include header igstkMacro and igstkEvents (fixed)
• Fix typo in documentation and comments (fixed)
• No test for this class (fixed)
• Add VesselObject(const Self&) and void operator=(const Self&) method to enforce smart pointer standards (fixed);

Luis' Comments

• Constructor and destructor do not need "void" as argument. (fixed)
• Test is missing : Code coverage = 0% (fixed)
• SetVesselSpatialObject is not protected by state machine logic. (fixed)
• State machine is not programmed. (fixed)

USImageReader

Andinet's Comments

• code style issues (Fixed)
• 0% code coverage (fixed)

Luis' Comments

• Documentation says that the class verifies the modality of the DICOM image. This is not implemented yet. (documentation fixed)
• WARNING: The same bug exists in the CT and MRI readers. This is a critical bug and must be fixed as soon as possible.

USImageObject

Andinet's comments

• The class documentation is not clear. Although the class is named "USImageObject", the documentation doesn't say anything about ultrasound images. (fixed)
• 0% code coverage (fixed)

Luis' Comments

• Clean style
• Just added Doxygen reference (see also) to the MRImageSpatialObject

USImageRepresentation

Andinet's comments

• 0% code coverage (fixed)

Luis' Comments

• Full Doxygen documentation missing. (fixed)
• Doxygen Group was non existant. (something to review after the release, we need to update a Modules.dox file). Doxygen groups are identified by a single word. A class may belong to multiple groups, but each one of them is identified by a single word. It seems that we have not created a central file with the list of groups.
• Moving empty declaration of the destructor from the .h file to the .cxx file. (fixed)
• Missing copy constructor and assignment operator declarations in the private section. (fixed)

USImageSimulator

Luis' Comments

• Lacks full Doxygen documentation. (fixed)
• Observer that looks for events from an ImageSpatialObject should be private.
• Typedefs that are not needed for the public API, must be declard in the private/protected section as appropriate.
• Unecessary includes removed:
  • "vtkImageActor.h"
  • "vtkLookupTable.h"
  • "vtkImageMapToColors.h"
• BUG: Calling a Request method in the constructor before having programmed the State Machine.
• Missing member variables in PrintSelf().
• Initialization and constructions of member variables in the Constructor MUST be done before programming the state machine.

ToolCalibration

David's comments

• looks good

Luis' comments

• Brief doxygen comment lack terminal period. (fixed)
• Missing full Doxygen comment (fixed)
• Doxygen group should include "Calibration" (fixed)
• GetCalibrationTransform is unsafe it must be replaced with a payload event. (not worth to modify at this point).
• Accepting "char *" in the Request methods is dangerous (why is the API not using only std::strings passed by reference ?). Pointers to chars do not guarantee to have a NULL terminator and are prone to dangling pointers.
  • Using a different type for variables and "variables to be set" is misleading and breaks the style of the rest of the toolkit.
  • (Fixed) Now using std::strings
• Unit Test Missing (fixed: test was added).

ToolCalibrationReader

David's comments

• looks good, a very good base class

Luis' comments

• Lacks full doxigen documentation.
• GetCalibration method uses a pointer. It should be replaced in the future with a loaded event.
• Member variables should be private instead of protected.
  • m_FileName
  • m_Calibration
• The method RequestSetFilename of the pivotToolCalibrationReader should be at the level of the generic ToolCalibrationReader.
• GetError() should use a reference to a float instead of a pointer to a float, even better it should send data in an event.
• GetParameter( should use a reference to a float instead of a pointer to a float, even better it should send data in an event.
• GetError() and GetParameter() should use std::strings instead of (char *) for the names of the arguments.
• Typedef for types that are only private should also be declared private.
• Printself is missing all the member variables.
• Programming of the state machine is missing. (it must be moved from the pivot calibration.

PivotCalibrationReader

David's comments

• looks good

Luis' comments

• The State Machine must be moved to the superclass, since all of it is not Pivot dependent.
• Root mean square error was missing from the PrintSelf method.
• The interaction with the Friend class needs to be reorganized, according to changes in the new base class the ToolCalibrationReader.