[Insight-users] Issues and possible bugs with Transforms etc

[Gavin Baker]

Hello,

I've been doing some digging around in a few corners of the transforms and
MI registration code, and have found a few issues and possible bugs in the
process.


Part I: Transforms


1. Euler2DTransform has its own m_Angle data member, which shadows the one
   declared in its superclass Rigid2DTransform.  Suggest making m_Angle in
   itkRigid2DTransform protected, then Euler2DTransform can access it
   directly, then remove the duplicated member.

2. Euler2DTransform::SetRotation() does (almost) the same as
   itkRigid2DTransform::SetAngle(), but calls its own ComputeMatrix()
   instead of overriding the virtual ComputeMatrixAndOffset() in its super.
   This seems redundant, especially if it properly overrode
   ComputeMatrixAndOffset().  Suggest removing SetRotation().  Only one or
   two other classes call this directly.

3. Euler2DTransform::ComputeMatrix() does exactly the same as the first half
   of the super's virtual itkRigid2DTransform::ComputeMatrixAndOffset().  It
   could use this except we don't want to clobber m_Offset.  Maybe refactor
   Rigid2DTransform and split calculating rotation matrix from offset?

4. Bug!  Rigid2DTransform needs this added to the default ctor to properly
  initialize stuff:

    m_Angle = NumericTraits< TScalarType >::Zero;
    m_Center.Fill( 0.0 );
    m_Translation.Fill( 0.0 );

  It would actually be better to implement the default ctor in terms of the
  other, passing the default params over to the super's ctor.

5. Bug!  All the BackTransform()s in Rigid2DTransform use m_InverseMatrix
   directly.  But it is only calculated in GetInverseMatrix(), and
   ComputeMatrixAndOffset() doesn't update it, even though it touches the
   MTime.  So any time a call is made that changes the parameters but doesn't
   force the inverse to be computed will be using the previous value.  For
   example:

    xform->SetAngle(phi);
    // now rotation matrix and offsets are updated, but not inverse
    xform->TransformPoint(point1);              // This will be correct
    xform->BackTransform(point2);               // but this won't

  Suggest using GetInverseMatrix() in the BackTransforms(), or just forcing
  a recalc in ComputeMatrixAndOffset().


Part II: ModelImage registration


1. Need MovingSpatialImage in ImageToSpatialObjectMetric to be non-const
   pointer.  It is perfectly valid to call certain non-const methods on the
   SO within the metric.

2. The ImageToSpatialObjectRegistrationMethod::Initialize() should validate
   parameter length with the metric, not the transform.  This will allow
   sneaky people such as your humble author to tack extra parameters onto
   the end of those intended for the transform, so the metric can adjust
   other (non transform) stuff.  In the default case, the metric will still
   have the same number as the transform, so it is backwards-compatible.
   Suggest:
  
  if ( m_InitialTransformParameters.Size() != m_Metric->GetNumberOfParameters() )



Part III: Miscellaneous


1. The ArrowSpatialObject has length spelled "lenght" everywhere.  Suggest
   it be attacked with sed.  It appears only the MetaArrowConverter has a
   dependency on this.


That's all for now... I can provide patches for virtually all of the above,
with the blessing of someone from the dev group.  I'm just a bit nervous
about changing something that has the potential to break everyone's
registration code!  :D

Comments?

ciao -

  :: Gavin

-- 
Gavin Baker                                      Complex Systems Group
http://www.cs.mu.oz.au/~gavinb             The University of Melbourne