[Insight-developers] Re: FlatStructuringElement

[Luis Ibanez]

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
>