[Insight-developers] Modes versus Objects
Gaëtan Lehmann
gaetan.lehmann at jouy.inra.fr
Thu Jun 2 15:21:33 EDT 2011
Luis,
In the context of the BinaryFunctorImageFilter, the duplicated code
would be all the subclasses that would have to be duplicated do get
another superclass able to work in physical space.
For the constant feature, I've done it to avoid having
DivideImageFilter, DivideByConstantImageFilter and
DivideConstantByImageFilter, and the other similar filters for which
it makes sense to have an image and a constant as operand.
It was also a way to clean some classes from the Review directory:
they were not needed anymore.
But maybe this is not the right approach. Maybe the wrong thing is to
have those subclasses.
We may want to drop them; then, depending on what we want to do, we
can use
typedef itk::BinaryFunctorImageFilter< ImageType1, ImageType2,
ImageType3, itk::Functor::Add2< PixelType1, PixelType2, PixelType3 > >
AddFilterType;
...
or
typedef itk::PhysicalSpaceBinaryFunctorImageFilter< ImageType1,
ImageType2, ImageType3, itk::Functor::Add2< PixelType1, PixelType2,
PixelType3 > > AddFilterType;
...
or
typedef itk::Constant1BinaryFunctorImageFilter< ImageType2,
ImageType3, itk::Functor::Add2< PixelType1, PixelType2, PixelType3 > >
AddFilterType;
...
or
typedef itk::Constant2BinaryFunctorImageFilter< ImageType1,
ImageType3, itk::Functor::Add2< PixelType1, PixelType2, PixelType3 > >
AddFilterType;
...
No code duplication, no code creep!
We can drop AddImageFilter and other similar filters.
Of course that would be a bit more difficult to wrap, but nothing
undoable.
That being said, I'm quite sure no one ever do
typedef itk::BinaryFunctorImageFilter<ImageType, ImageType,
ImageType, itk::Functor::Add2< PixelType, PixelType, PixelType > >
AddFilterType;
...
even if that's possible with ITK 3.20, because it is easier to just do
typedef itk::AddImageFilter<ImageType, ImageType, ImageType>
AddFilterType;
...
Wanting something pleasant to use seems quite natural.
To go back to Bill's mail, for the base classes, there is a trade off
between code duplication and mode.
And usability should be added also.
Regards,
Gaëtan
Le 2 juin 11 à 19:19, Luis Ibanez a écrit :
> 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
>>
>>
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 203 bytes
Desc: Ceci est une signature ?lectronique PGP
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20110602/436465a8/attachment.pgp>
More information about the Insight-developers
mailing list