[Insight-developers] Re: FlatStructuringElement

[Gaëtan Lehmann]

Luis,

This class is a big improvement for the wrappers, because it let us have  
only one structuring element class for all the structuring element shapes  
- an important feature given that this class is used as template parameter  
for most of the mathematical morpholgy filters, and given the space (and  
time) needed to multiply the number of filter instantiations.
I have imagined this class for the wrappers a long time before it finally  
get implemented to support the new morphology algorithms - it was just a  
very good reason to break the backward compatibility.

This class is not limited to binary morphology. It is usable with  
grayscale morphology as well. The current use of structuring element in  
ITK is *wrong*: the structuring element don't need to have the same pixel  
type than the image on which it will be used to perform an erosion, a  
dilation, or something else. All what the structuring element need to  
define is: this neighbor is in the neighborhood, and this neighbor is not  
in the neighborhood - a binary information perfectly carried by a bool.  
Using a structuring element independant of the image type allow us to use  
the same structuring element with different image type. Again, it decrease  
the complexity of the toolkit.

I have called this class BinaryStructuringElement at first, but Richard -  
a mathematical morphology expert, and a native english speaker - was  
thinking that Flat was better than Binary. Flat structuring element is  
frequently used in mathematical morphology related texts - binary  
structuring element seems to be far less frequent.

Do you know I lost a week of work, a year ago, trying to trace why the  
erosion (or dilation, I don't remember) was wrong in python, only because  
I have forgoten to call CreateStructuringElement() ? Quite incredible but  
true. I have even created a account to Brad on my system to get some help.
I have also received several emails from people making the same mistake  
since that.
Without the need to call CreateStructuringElement(), or with a GetImage()  
mehtod (to visualize (yes, visualize) the structuring element, after have  
wrote it to a file, or passed it to vtk, or something else), nobody would  
has been stopped like I was.

I have also thought to a generator class, but it looked like a new  
increase of complexity. The templated method looked like a much simple way  
to go, and there was already some examples of templated method in ITK.
The methods with the "UC" suffix are a tempoary fix, to workaround a  
cableswig deficiency, like the GetElement() method in the Array class (and  
others).
It doesn't make the Image type a major parameter, because the image type  
is only used for 2 methods not fundamentaly related to that class: they  
are only there to convert the structuring element from/to an image.

This class was added to keep the backward compatibility in the future  
releases. I have announced my intention when we were talking about moving  
WrapITK to ITK, a month ago, and you replied me:

    "If you are planning to modify WrapITK's API, it will be great to
    do that before we commit it into ITK, or at least before we release
    ITK 3.0"

I have even sent some links to the FlatStructuringElement code in my  
email. Where were all your comments at this time, or when I sent the email  
asking for review to the developer list ? Why haven't you told me it was a  
bad idea at this time ?
I have made the changes before committing WrapITK to ITK, so we can have  
as much time as possible to test. Why do I get feedbacks only now ? Why do  
I have to read the code to know that you think the design is wrong ?

Your changes have made the dashboard more red than before: WrapITK even  
doesn't compile on my host. It seems you don't get all the consequences of  
what your are changing in WrapITK. That's really *normal*: WrapITK is huge  
and complex.
So why don't you ask ? I'm generally replying quickely to my emails...


   Regards,


      Gaetan



Le Sun, 01 Oct 2006 20:50:48 +0200, Luis Ibanez <luis.ibanez at kitware.com>  
a écrit:

>
>
> Hi Gaëtan
>
>
>    Yes,
>
>
>        I have been changing the FlatStructuringElement...
>
>
>
>    This class has never been compiled on Windows before and it
>    needed a serious refactoring.
>
>
>
>    This class should have *never been committed* to ITK in the
>    first place.
>
>
>    It got smuggled as part of the addition of Wrap ITK. That's not
>    the normal process for adding classes. Should we have followed
>    the process, we could be discussing the API in the Journal.
>
>
>    This class is not part of WrapITK at all. The fact that it was
>    committed in the top directory of the WrapITK CVS repository is
>    actually quite surprising. The class is completely unrelated to
>    the wrapping process.
>
>
>    Dealing with the deficiencies of this broken class have cost so
>    far *two weeks* of red Dashboards and have prevented us from
>    checking whether WrapITK works or not in Windows with VisualStudio
>    6.0, 7.0, 7.1, 8.0.
>
>
>    At this point we can not yet tell whether WrapITK is a realistic
>    alternative for the Wrapping in ITK. The fact is that it has not
>    been demostrated that WrapITK can be used in Windows.
>
>
>
>    We don't have a single example of a successful build of WrapITK
>    on Windows and we are *29 days* before including it in a release.
>
>
>
>    I suggest we remove this FlatStructuringElement class and we use
>    our time in making sure that WrapITK actually works on Windows,
>    before we take the risk of shipping it with ITK 3.0.
>
>
>
>
> -----------------
>
>
>
>
>    About the FlatStructuringElement class:
>
>
>    We can understand that you are not a VisualStudio user, that's fine,
>    but ITK is committed to support multiple platforms and that commitment
>    result in finding compromises on coding techniques.
>
>
>    I read the paper that you posted to the IJ, but unfortunately
>    it is quite incomplete:
>
>                http://hdl.handle.net/1926/308
>
>
>    There is only abstract paragraph, and introduction paragraph,
>    and a section with the list of methods in the class, the rest
>    of the paper only contains subtitles.  It is definitely too
>    incomplete for using is as a base for discussion. That was
>    certainly a premature submission.
>
>
>    The argument raised in the paper seems to be that calling the
>    method:
>
>                 "CreateStructuringElement()"
>
>
>    is too complicated...
>
>
>    and that as a solution you provide a new class templated over
>    dimension with templated methods (over the image type), and add
>    to it hard coded methods for Unsigned Char images, that can be
>    used from the wrapping.
>
>
>       I'm not convicend that the quest for simplicity has been
>       successful here...
>
>
>    The argument, made in the introduction of the paper, that ITK doesn't
>    provides methods for visualizing structuring elements is rather out
>    of basis, because ITK does not provide *any* visualization at all.
>
>
>    It is very clear that ITK was never intended for providing
>    Visualization or GUI functionalities.
>
>
>    The combination of templates over Dimension and over image type
>    is redundant, specially because your image already carries dimension
>    information, and if somebody attempts to mix dimensions, they will
>    result in an inconsisten structuring element.
>
>
>    The name of the class is ambiguous. "Flat" leads to think that
>    this class only works in "2D'.
>
>
>    I agree with you that there is no need for GetImageUC methods.
>    They actually break the style of ITK. Those methods were there
>    in the original design of the class, and I just keep them there
>    while trying to get it to compile in other platforms. Those methods
>    should be removed.
>
>
>    I disagree that the pixel type is not a mayor parameter of the
>    class. You are already using the Image type as parameter for the
>    templated methods (that Visual studio does not support quite well).
>    You have mixed up the concept of Structuring Element, and Structuring
>    Element Generators, in a single class. That conceptual mix up is the
>    source of the unnecessary complexity of this class. You could have
>    created a StructuringElement (limited to BinaryMorphology) by using
>    bool pixel types and only dimension as a template, and apart from
>    it you could have created Generator classes that build Structuring
>    elements out of an image, or that create an image out of an
>    structuring element.
>
>
>
>    In any case, let's separate the task of adopting WrapITK, which is
>    *HUGE* change, from the problem of including a class that provides
>    a marginal improvement over functionality currently existing in
>    the toolkit.
>
>
>
>
>       Regards,
>
>
>
>        Luis
>
>
>
> -----------------------
> Gaëtan Lehmann wrote:
>>  Hi Luis,
>>  I see you have done some changes in the FlatStructuringElement code.
>> I'm afraid those changes are breaking some features of the class, and  
>> are  making some strange things:
>> - FromImageUC() is declared twice, with a different number of args.  
>> This  was made because with wrappers, the default values are not used,  
>> and we  must give a value, even if a default value exist in the  
>> c++ code.
>> - Same thing for GetImage().
>> - If FromImage() and GetImage() are not templated, there is no need  
>> for  FromImageUC(), and GetImageUC()
>> - The pixel type is not a major parameter for this class. Only the   
>> dimension is important. Giving the Image type used only to create a   
>> structuring element from an image, and to produce an image from a   
>> structuring element, will confuse the user who will think that's the  
>> type  of Image on which the structuring element can be used. Can't we  
>> keep the  templated methods GetImage() and FromImage() ? There is some  
>> other  templated methods in ITK, like the   
>> TransformContinuousIndexToPhysicalPoint() and   
>> TransformIndexToPhysicalPoint() methods of the Image class. At least,  
>> it  should be the second template parameter, and should get a default  
>> value  (Image< unsigned char, VDimension).
>>  Have you read the description submited to the insight journal ? It  
>> surely  not in very good english, but should describe clearly enough  
>> some of the  problems above.
>> http://insight-journal.org/dspace/bitstream/1926/308/2/Consolidated_morphology.pdf
>
>>  Please let me know what you want to do.
>>  Gaetan
>>
>
>



-- 
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