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

Gaëtan Lehmann gaetan.lehmann at jouy.inra.fr
Tue Nov 16 17:52:28 EST 2010 

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

-------------- 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/20101116/3751db42/attachment.pgp>

More information about the Insight-developers mailing list