ITK/Release 4/Modularization/Code Reviews/Checklist

From KitwarePublic
Jump to navigationJump to search

This page defines the check list to be used by reviewers during the process of reviewing the files in ITKv4.

Major Topics

The reviewer will verify the file for compliance with the following major topics

  • Coding Style
  • Const correctness
  • Mini-Pipelines
  • Progress Reporting
  • Multi-Threading
  • Member variables holding to memory

The following sections specify the details to be evaluated with each one of these major topics.

Coding Style

Despite the fact that coding style is verified regularly with KStyle, there are still many details that escape that verification. The following list specifies what elements must be verified by the reviewer.

  • Filename must match class name
  • Files must contain a single class
  • Implementation of methods must be in the .hxx or .cxx files, not the .h files
  • All files must have the Copyright Header at the top.
  • #define for class name in the .h and .hxx files. __classname_h and __classname_hxx
  • Doxygen documentation
    • Brief class doxygen description
    • Complete class doxygen description
    • make sure there is: \ingroup ITKModuleName (ModuleName matching the module it belongs, else ModuleInDoxygenGroup will fail)
  • namespace itk
  • Classes that use SmartPointers must have
    • Constructor/Destructor protected
    • Copy Constructor : private and not implemented
    • Operator= : private and not implemented
  • No acronyms in class name or method names
  • No unnecessary headers #included
  • Justify every public method
  • All member variables must be private
  • Use Set/Get Macros (macros should call Modified() to ensure pipeline data is updated)
  • If deriving from itkObject
    • Use New macro
    • declare SmartPointer<T> as Pointer
    • declare SmartPointer<const T> as ConstPointer
    • declare Self
    • declare Superclass
    • use TypeMacro
    • Have PrintSelf() method and print all the member variables
  • 100% code coverage
    • Run Utilities/Maintenance/computeCodeCoverageLocallyForOneTest.sh
    • Check dashboard
  • Any information that is printed or displayed has to be legible to human eyes
  • Respect Coding Style as specified in the document Insight/Documentation/Style.pdf
  • Must pass the test of KWStyle
  • [Style] In the .hxx files of template classes, the first 5 lines in the declaration of methods should be:
    • 1) template
    • 2) return type
    • 3) class name
    • 4) ::method name
    • 5) the opening bracket

For example: 

 template <  template_arguments >
 return_type
 classname< template_arguments >
 ::methodsName( method_arguments )
 {
   // indentation of two spaces
   // for the first line of code
   int k = 0;
 }

In many cases line (2) uses a trait from the templated class and therefore becomes: 

 typename classname< template_arguments >::trait

Running KWStyle

In a single file

  KWStyle -xml ITK.kws.xml itkQuadEdgeMeshTest1.cxx -v

In all the repository

  make StyleCheck

Const Correctness

  • Get method must be const
  • SetInput methods must take "const raw pointers" as input
  • GetOutput methods must give const raw pointers as output
  • All non-trivial types must be passed by reference (except for DataObjects, that must be passed by const raw pointers).
  • const_cast must only be used inside of the SetInput() method, to overcome the lack of const-correctness in the pipeline.
    • overriding GenerateInputRequestedRegion requires const_cast on the inputs
    • any other uses of const_cast must be scrutinized

Mini-Pipelines

Filters that implement a mini-pipeline with N internal filters should

  • Examine that this method is consistent with all filters in mini-pipeline:
  void EnlargeOutputRequestedRegion(DataObject *output)
  • Call ReleaseDataFlagOn() in the first N-1 filters of its mini-pipeline
  • For the filters that can run in-place, set them to do so by calling InPlaceOn(). Except the first filter.
  • Use the ProgressAccumulator in order to compose a progress report from each one of the internal filters
  • Use GraftOutput() in the following way at the end of the GenerateData() method
 caster->GraftOutput( outputImage );
 caster->Update();
 this->GraftOutput( caster->GetOutput() );

Region Management

All Filters (supporting streaming or not)

  • In their GenerateData() method they should:
    • Only process the Requested Region of the output
    • Never access more than the Buffered region of the input


If a filter does not support streaming it should:

  • Override EnlargeOuputRequestedRegion, and call SetRequestedRegionToLargestPossibleRegion on it's input
  • It does not need to override the GenerateInputRequestedRegion
    • unless the filter is manipulating the size, or geometry

Streaming

Filters that are intended for supporting streaming must consider that:

  • The RequestedRegion can be smaller than the LargestPossibleRegion.

As such, the boundary of the output BufferedRegion may not be a boundary condition and thus the filter writer must implement GenerateInputRequestedRegion to ensure that any pixels outside the output BufferedRegion that are within the LargestPossibleRegion that are needed for proper computation are added to the input RequestedRegion.

Progress Reporting

Filter should report

  • ProgressEvents or
  • IterationEvents

If they report ProgressEvents, they should use the ProgressReporter class as 

   ProgressReporter progress( this, threadId, outputRegionForThread.GetNumberOfPixels() );
   
   while( ... )
     {
     // do stuff
     progress.CompletedPixel();
     }

If the filter implements a mini-pipeline, then the progress reporting should be done using the ProgressAccumulator inside the GenerateData() method as: 

 ProgressAccumulator::Pointer progress = ProgressAccumulator::New();
 progress->SetMiniPipelineFilter(this);
 
 progress->RegisterInternalFilter( internalFilter01, fraction1 );
 progress->RegisterInternalFilter( internalFilter02, fraction2 );
 progress->RegisterInternalFilter( internalFilter03, fraction3 );
 progress->RegisterInternalFilter( internalFilter04, fraction4 );


where internalFilterX is each one of the filters in the mini-pipeline, and fractionX are number that add up to 1.0, and represent the fraction of time that each one of the filters will contribute to the total computation. This fraction is assigned as an estimation, and doesn't have to be perfect, just aim for the correct order of magnitude.


Multi-Threading

Filters that are multi-threaded, must implement the method: 

  void ThreadedGenerateData(
     const OutputImageRegionType & outputRegionForThread,
     ThreadIdType threadId)

and can (but do not have to) implement the methods 

   void BeforeThreadedGenerateData()
   void AfterThreadedGenerateData()

These two methods are intended to implement the preparation of variables that host computations on every thread, and to consolidate that information once the threads have completed their work.

The filter should not implement the GenerateData() method, unless it needs anything different from what is provided in the default method in 

     ImageSource::GenerateData()

that essentially does: 

  void GenerateData()
  {
  this->AllocateOutputs();
  this->BeforeThreadedGenerateData();
  
  ThreadStruct str;
  str.Filter = this;
  
  this->GetMultiThreader()->SetNumberOfThreads( this->GetNumberOfThreads() );
  this->GetMultiThreader()->SetSingleMethod(this->ThreaderCallback, &str);
  
  this->GetMultiThreader()->SingleMethodExecute();
  
  this->AfterThreadedGenerateData();
  }

Any variables that need to be written into during the execution of the ThreadedGenerateData() method, must be put into an array, to provide an independent instance to every thread. This preparation can be done in the BeforeThreadedGenerateData() method, and the consolidation of values in the arrays can be done in the AfterThreadedGenerateData() method.

Example

For an example, look at the classes

  • itkStatisticsImageFilter
  • itkUnaryFunctorFilter

Member Variables

Filters should not use member variables to hold to their inputs or outputs.

The inputs and outputs should be maintained by the methods 

   SetInput(const InputImageType *input)
     {
     this->ProcessObject::SetNthInput( 0,const_cast< InputImageType * >( input ) );
     }



  GetInput( unsigned int idx ) 
    {
    return static_cast< const TInputImage * >( this->ProcessObject::GetInput(idx) );
    }



  GetOutput()
    {
    if ( this->GetNumberOfOutputs() < 1 )
      {
      return 0;
      }
    
    return static_cast< TOutputImage * >( this->ProcessObject::GetOutput(0) );
    }


In most cases, these methods are already provided by super classes. In particular by 

  * itk::ImageSource
  * itk::ImageToImageFilter


Notes


ITK: [Welcome | Site Map]
Retrieved from "https://public.kitware.com/Wiki/index.php?title=ITK/Release_4/Modularization/Code_Reviews/Checklist&oldid=51385"