[Insight-developers] itkResampleImageFilter.h & itkWarpImageFilter.h and Software Archeology

[Luis Ibanez]

Stephen, Dan, Bill,


We are digging into the past of the

           itkWarpImageFilter and itkResampleImageFilter

in order to understand how to fix current issues with the API of the
WarpImageFilter.


In particular:


Stephen
========

We are discussing this change that you made on the ResampleImageFilter:
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkResampleImageFilter.h?root=Insight&r1=1.55&r2=1.56

and we are wondering why was it needed.

It seems that the correct fix should have been to make the argument of
the method

              void SetOutputParametersFromImage()

to be a const pointer to an images.

We don't have anywhere in the toolkit any method that requires its
name to be duplicated just to support a const argument.

Normally this is managed simply by overloading the method with a
const signature. For example

    void SetOutputParametersFromImage( ImageType * image )
    void SetOutputParametersFromImage( const ImageType * image )

and.. .in this particular case, the method doesn't need to call any
non-const method from the input image, so there is no reason for
needed the method with the first signature (the non-const one).



Bill
=====

Looking further into the history of this method, we find this
change
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkResampleImageFilter.h?root=Insight&r1=1.49&r2=1.50

Which is a very deep modification of the use of SetReferenceImage method.

Why was this change needed ?



Dan
====

You added the SetOutputParametersFromImage() method in 2005:
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkResampleImageFilter.h?root=Insight&view=diff&r1=1.35&r2=1.36

Why didn't you used:

    a) a raw pointer for the image, and
    b) a const raw pointer (given that the image
         argument doesn't need to change ?)


Bill,
=====

You added the SetReferenceImage() method
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkResampleImageFilter.h?root=Insight&r1=1.37&r2=1.38

but we already have the SetOutputParametersFromImage() method at that point.



------

  If Backward-Compatibility wasn't a factor, I will suggest

     - Remove the SetOutputParametersFromConstImage() method.
       There was no reason for adding such method, when the
       same effect can be achieved by simply using a const raw
       pointer in the SetOutputParametersFromImage() method.

     - Make the SetOutputParametersFromImage() take a const
       raw pointer (not a smart pointer).

     - Solve the ambiguity and duplication of functionality
       between the SetReference() image and the
       SetOutputParametersFromImage() methods.


   I'm not sure that we can clean up this in a way that
   is consistent at this point...

   We could deprecate the ConstImage() method, but since deprecation
   doesn't imply removal, we have to maintain it anyways...


------



   Being positive and looking to the Future:
   ==========================================


   It looks like we *MUST* implement some sort of
   *systematic* code-review practice.

   I'm looking at [http://www.review-board.org/] as an option.



        Luis