[Insight-developers] itk::Index, itk::Offset, ... as subclass of itk::FixedArray?

Gaëtan Lehmann gaetan.lehmann at jouy.inra.fr
Wed Nov 17 03:25:35 EST 2010 

Le 17 nov. 10 à 00:46, Bill Lorensen a écrit :

> I'm still confused. We have had wrapping for years. What has changed
> to bring this problem to light? I admit that I have never use ITK
> wrapping.

It has been broken for years. It wasn't the biggest problem in  
wrapping though, and was quite difficult to fix, so it stayed broken.
Now that the wrapping is a lot better, and that we have a chance to  
fix things without being too much constrained by the backward  
compatibility, we have to take care of that problem.

>
> Please don't confuse the issue with method names in filters that are
> inconsistent. I'm sure we have lots of those.

I'm not. I only wanted to say that fixing the consistency problem may  
help to keep the backward compatibility.
Sorry if it wasn't clear.

>
> 2010/11/16 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>:
>>
>> Le 16 nov. 10 à 22:59, Bill Lorensen a écrit :
>>
>>> Gaëtan,
>>>
>>> I'll bet we can find a backward compatible solution. I'll try to  
>>> find
>>> some time to look into it. Better we spend time than our customers.
>>>
>>> Remind me again the important bug/issue you are trying to resolve.
>>
>> The principal problem is for the wrapping: those methods are simply  
>> not
>> usable. This is something which must be fixed for ITK v4.
>>
>> This is because we don't know what the pointer is for. Is it an  
>> array? Of
>> which size? It is a pointer to a buffer? Is it a pointer to a  
>> modifiable
>> variable?
>>
>> Using stronger types, as done everywhere else in ITK, doesn't  
>> exhibit this
>> kind of problem.
>> Stronger types can also be more efficiently checked at compile time  
>> -- I'm
>> really not sure why they haven't been used in the first place!
>>
>> One option to avoid the compatibility issue is to use another  
>> method name.
>> Of course it won't be always possible to find another meaningful  
>> name, but
>> it may be useful in some cases. For example, in PadImageFilter, the  
>> method
>> names are not consistent with the methods name in CropImageFilter.
>> Get/SetPadLowerBound() and Get/GetPadUpperBound() could be renamed to
>> Get/SetUpperBoundaryPadSize() and Get/SetLowerBoundaryPadSize() and
>> Get/SetPadLowerBound() and Get/GetPadUpperBound() reimplemented to  
>> use the
>> new methods and still provide the same API.
>>
>> The constructors similar to the ones in itk::FixedArray would also  
>> help a
>> lot, but not for return values.
>> I'll continue to look that way, because I think that having to deal  
>> only
>> with the return values, which are a lot less used, is acceptable.
>>
>> Gaëtan
>>
>>
>>>
>>> Bill
>>>
>>>
>>> 2010/11/16 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>:
>>>>
>>>> Le 16 nov. 10 à 22:25, Bill Lorensen a écrit :
>>>>
>>>>> The backward compatibility issue is not good. We need a better  
>>>>> solution.
>>>>
>>>> IMO, using C arrays was wrong and must be fixed. It could have been
>>>> worth,
>>>> but the C arrays are used in a quite small number of places.
>>>>
>>>> I'd like to enhance the backward compatibility - that's why I'm  
>>>> looking
>>>> for
>>>> better constructors for Index Offset and Size - but I doubt a fully
>>>> backward
>>>> compatible solution can be found, especially for the return values.
>>>>
>>>> I may also not found the best approach...
>>>>
>>>>>
>>>>> 2010/11/16 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>:
>>>>>>
>>>>>> Le 16 nov. 10 à 16:56, Tom Vercauteren a écrit :
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> On Tue, Nov 16, 2010 at 16:35, Luis Ibanez <luis.ibanez at kitware.com 
>>>>>>> >
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi Gaetan,
>>>>>>>>
>>>>>>>> Probably only historical reasons...
>>>>>>>>
>>>>>>>> I don't see why we couldn't make the itk::Index and
>>>>>>>> the itk::Offset be derived classes of the itk::FixedArray.
>>>>>>>>
>>>>>>>> Note however that Dave Cole is working hard on
>>>>>>>> fixing the Windows 64 bits issues, so you may want
>>>>>>>> to coordinate those changes (since the itk::Index and
>>>>>>>> the itk::Offset are at the heart of the 64bits problem).
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> Regarding the intentional lack of a default constructor,
>>>>>>>> we may have to run these experiments again.  I would
>>>>>>>> suspect that it is still a valid issue, given that this relates
>>>>>>>> to making the itkFixedArray appear as plain-old-data,
>>>>>>>> instead of as a class. (there was also an issue of padding
>>>>>>>> if I remember correctly...)
>>>>>>>
>>>>>>> Indeed, currently it is still easy to access the memory help  
>>>>>>> by an
>>>>>>>  itk::Image< itk::Vector<float,2>, 2 >
>>>>>>> by a simple
>>>>>>>  float * floatptr = image->GetBufferPointer() 
>>>>>>> [0].GetDataPointer()
>>>>>>>
>>>>>>> Not sure if it is standard conformant since itk::Vector seems  
>>>>>>> to be
>>>>>>> only POD like and not strictly POD but at least it works on the
>>>>>>> systems I have tested so far. This is important to avoid copying
>>>>>>> memory between different libraries.
>>>>>>>
>>>>>>> This works because itk::Vector inherits from itk::FixedArray  
>>>>>>> and is
>>>>>>> also POD-like. The less POD it becomes the more fragile my  
>>>>>>> hypothesis
>>>>>>> becomes. Remember that we had to remove an empty destructor to  
>>>>>>> make
>>>>>>> itk::FixedArray closer to being POD. This led to a better memory
>>>>>>> alignment and improved the performance of some of our code by  
>>>>>>> a factor
>>>>>>> 2:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> http://www.itk.org/mailman/private/insight-developers/2008-June/010444.html
>>>>>>>
>>>>>>> Note that I am not the only one caring about this:
>>>>>>> http://www.itk.org/pipermail/insight-users/2010-October/038525.html
>>>>>>>
>>>>>>> Anyhow, we may want to improve the documentation of  
>>>>>>> itk::FixedArray
>>>>>>> with respect to that point.
>>>>>>>
>>>>>>
>>>>>> ok, so itk::FixedArray is the class tweaked for efficiency and  
>>>>>> which
>>>>>> act
>>>>>> like POD.
>>>>>> And itk::FixedArray, unlike itk::Index, itk::Offset or  
>>>>>> itk::Size, also
>>>>>> provide the constructor I'm looking for.
>>>>>>
>>>>>>  http://www.itk.org/Doxygen318/html/classitk_1_1FixedArray.html
>>>>>>
>>>>>> So I guess it should be possible to implement those 3 classes as
>>>>>> subclasses
>>>>>> of FixedArray, or at least to implement the same kind of  
>>>>>> constructor
>>>>>> for
>>>>>> those classes.
>>>>>>
>>>>>>> My two cents,
>>>>>>> Tom
>>>>>>>
>>>>>>>>
>>>>>>>> What would be the argument in favor of adding the
>>>>>>>> default constructor ?
>>>>>>>>
>>>>>>>> (initialization ?,
>>>>>>>> note that we don't know what T is, in advance...
>>>>>>>> I'm not sure that we can provide a reasonable
>>>>>>>> initialization anyways.  T could be a std::string,
>>>>>>>> or a highly complex class...)
>>>>>>
>>>>>>  Backward compatibility.
>>>>>>
>>>>>> This is related to this change:
>>>>>>
>>>>>>  http://review.source.kitware.com/#change,374
>>>>>>
>>>>>> I had to modify a lot of tests to make it build. For example:
>>>>>>
>>>>>>
>>>>>>
>>>>>>  http://review.source.kitware.com/#patch,sidebyside,374,1,Testing/ 
>>>>>> Code/Algorithms/itkNormalizedCorrelationImageMetricTest.cxx
>>>>>>
>>>>>> In that test, only the size parameter break, because itk::Size  
>>>>>> doesn't
>>>>>> provide the right constructor. The other parameters I have  
>>>>>> changed like
>>>>>> the
>>>>>> spacing or the origin are not breaking anything, I think  
>>>>>> because their
>>>>>> type
>>>>>> is derived from itk::FixedArray
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Gaëtan
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>   Luis
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ------------------------------------------------------------------------
>>>>>>>> 2010/11/16 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Is there a reason to have the basic array types itk::Index and
>>>>>>>>> itk::Offset, and maybe others, not implemented as a subclass  
>>>>>>>>> of
>>>>>>>>> itk::FixedArray?
>>>>>>>>> Also, I see in the doc that no constructor are provided for
>>>>>>>>> performance
>>>>>>>>> reasons. Is it still true with the compilers supported by  
>>>>>>>>> itk 4?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> 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
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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
>>>>
>>>>
>>
>> --
>> 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
>>
>>

-- 
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/20101117/7975c5d4/attachment.pgp>

More information about the Insight-developers mailing list