[Insight-developers] Modes versus Objects

Thu Jun 2 13:19:09 EDT 2011

Gaetan,

In the context of the BinaryFunctorImageFilter,
the code duplication that we are talking about is 20
lines of code in a while loop that iterates over the
two input images and store data in the output image.

See for example, that your recent changes in the
BinaryFunctorImageFilter for allowing the use of
constants instead of images falls also in this category
of something that should be better implemented in
a second class, than as a "mode" of an existing class.

That "duplication of code" was solved in that case
by actually "duplicating the code anyways" inside
of the filter itself and introducing an "if" (the root of
all evil) to switch between the two cases at run
time.  Plus introducing a error case in which the
filter should fail.   This is feature creep as well.

It looks like the conveniences of wrapping and
SimpleITK are percolating into degrading the
current implementation of ITK itself.

It is only at the wrapping level of use that a
developer may have to change her mind between
adding two images or adding an image to a constant.

Following the same "feature-thirsty" approach,
one could argue that now we want to have also the
option of using a SpatialObject instead of a constant
or an image, or also plug in another Functor class as
input.

It is all very cool,....

but soon, the inside of this filter will have a switch
statement with N different modes, bad code coverage
and poor verification of cross talks between the features.

So, now instead of three clean classes that each one
do one thing right,  with 100% code coverage, we have
a bloated class that has three modes of use:

 (image+image, image +constant, constant+image)

and that with Kent's changes will have additional submodes
for dealing with image resampling.

--------

Here is my main concern:

    We are wasting time.

    We are spending weeks fixing
    things that are not broken.

    We are lacking focus.

    It is June,

    We just postponed the Beta release because
    the expected work is not ready yet.

    We only have 6 months left.

    There are still 344 files in the Review
    module to be cleaned up.

    There are 2,300 files to be revised.

    There are multiple refactoring
    efforts to be completed.

    Is DICOM ready ?
    Has GDCM been fixed ?
    Are the GPU filters ready ?
    Is the FEM refactoring ready ?
    Is the Image Registration ready ?
    Are the LevelSet ready ?
    Is SimpleITK ready ?
    Where are the A2D2 ?

    We have *a lot* more to do than
    to spend the developers team time
    in fixing things that are not broken,
    just because they give a nicer look
    to some fringe types of use.

    Making the Add filter support constants,
    or having it perform resampling...

    ...that's not the focus that we should
    have in the ITKv4 effort.

       Luis

-------------------------------------------------
2011/6/2 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>:
>
> Le 2 juin 11 à 17:32, Bill Lorensen a écrit :
>
>> Folks,
>>
>> This discussion is motivated by recent activity...
>>
>> Back in the early days of OO programming, I recall having discussions
>> about the use of modes versus objects. When I taught OO courses I
>> always discouraged modes if they changed the "identity" of the object.
>>
>> Should a mode be used to change the behaviour of an object or should a
>> new object be created?
>>
>> A case where a mode is better than an object:
>> Controlling the visibility of an actor in VTK:
>>  1) Mode: vtkActor VisibilityOn/Off
>> or
>>  2) Object: two classes vtkActor and vtkVisibleActor
>>
>>  1) is the appropriate solution since, at run time, an application
>> may want to change the visibility of an actor.
>>
>> A case where an object may be better than a mode:
>>  1) Mode: itkShiftScaleImageFilter
>> SetOperationOrderToShiftScale()/SetOperationOrderToScaleShift()
>> or
>>  2) Object: two classes itkShiftScaleImageFilter and
>> itkScaleShiftImageFilter
>>
>> A VTK approach versus an ITK approach
>>  1) mode: vtkImageMathematics 21 different modes
>> (ADD,SUBTRACT,MULTIPLY,...)
>>  2) object: itkAddImageFilter, itkSubtractImageFilter,
>> itkMultipleImageFilter...
>>
>> It is not always obvious as to the best choice. But, I think the
>> question should be asked anytimne we introduce a new "mode".
>>
>
> Hi Bill,
>
> This is certainly a good thing to keep in mind while adding a new feature,
> and your example on ShiftScaleImageFilter and ScaleShiftImageFilter
> certainly make sense.
>
> But I'm not sure the decision will be that easy when it comes to base
> classes.
> Adding features in base classes may help to avoid duplicating the
> subclasses, and code duplication is certainly something we want to avoid.
>
> The BinaryFunctorImageFilter recently modified is a good example - do we
> want to have two versions of Add image filter, one for indexed data access
> and another for physical space data access?
>
> The InPlaceImageFilter is another good example. It allows a subclass to run
> in place or not. We probably don't want to duplicate the subclasses to
> implement that feature.
>
> It would be nice if everything can be simple!
>
> Gaëtan
>
>
> --
> Gaëtan Lehmann
> Biologie du Développement et de la Reproduction
> INRA de Jouy-en-Josas (France)
> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
> http://voxel.jouy.inra.fr  http://www.itk.org
> http://www.mandriva.org  http://www.bepo.fr
>
>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Kitware offers ITK Training Courses, for more information visit:
> http://kitware.com/products/protraining.html
>
> Please keep messages on-topic and check the ITK FAQ at:
> http://www.itk.org/Wiki/ITK_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://www.itk.org/mailman/listinfo/insight-developers
>
>