[Insight-developers] Casting constructors for DTI and Sym Mat pixeltypes
Luke Bloy
luke.bloy at gmail.com
Tue Mar 16 19:47:53 EDT 2010
Luis,
I do not have access to that compiler but i suspect the problem is that
I neglected to remove the non-templated constructor and assignment
operators.
I have just removed those methods and am running tests on my gcc box.
I can either commit the changes or if you prefer we can wait until
someone with a vs 6 box can test the patch.
Sorry for the inconvenience.
Luke
Luis Ibanez wrote:
> Hi Luke,
>
> The new code is failing in Visual Studio 6.0:
>
> http://www.cdash.org/CDash/viewBuildError.php?buildid=563721
>
> C:\Dashboards\My
> Tests\InsightContinuous\Code\Common\itkSymmetricSecondRankTensor.h(119)
> : fatal error C1001: INTERNAL COMPILER ERROR
>
>
> Do you have access to this compiler ?
>
>
> Please let us know,
>
>
> Thanks
>
>
> Luis
>
>
> --------------------------------------------
> On Tue, Mar 16, 2010 at 4:00 PM, Luke Bloy <luke.bloy at gmail.com> wrote:
>
>> Luis,
>>
>> The build was green, So I have just committed the patch. I'll keep my eye on
>> the continuous builds and the nightlies in the morning.
>>
>> Thanks for the help.
>> -Luke
>>
>> Luis Ibanez wrote:
>>
>>> Hi Luke,
>>>
>>> Thanks a lot for implementing the changes
>>> and preparing the patch.
>>>
>>> I tried to run it on my side,
>>> but the following file is missing:
>>>
>>> itkNumericTraitsDiffusionTensor3DPixel.cxx
>>>
>>>
>>> Could you please send this file ?
>>>
>>> ---
>>>
>>>
>>> The additional tests look reasonable.
>>>
>>>
>>> A good reference to have is to monitor the code coverage
>>> of the classes today, and verify that they have the same
>>> (or larger code) coverage tomorrow.
>>>
>>> To be more specific, we have today:
>>>
>>> itkSymmetricSecondRankTensor.txx : 98.80%
>>>
>>> http://www.cdash.org/CDash/viewCoverageFile.php?buildid=563343&fileid=10858202
>>>
>>> itkSymmetricSecondRankTensor.h : 100%
>>>
>>> http://www.cdash.org/CDash/viewCoverageFile.php?buildid=563343&fileid=10858201
>>>
>>> itkDiffusionTensor3D: 98.11%
>>>
>>> http://www.cdash.org/CDash/viewCoverageFile.php?buildid=563343&fileid=10857778
>>>
>>>
>>> ----
>>>
>>> If your Experimental build turns out to be green,
>>> please go ahead and commit the patch.
>>>
>>> Then,
>>> we should keep an eye on the continuous
>>> builds for the rest of the day.
>>>
>>>
>>> Thanks
>>>
>>>
>>> Luis
>>>
>>>
>>> --------------------------------------------------------
>>> On Tue, Mar 16, 2010 at 1:15 PM, Luke Bloy <luke.bloy at gmail.com> wrote:
>>>
>>>
>>>> Hi Luis,
>>>>
>>>> I'm attaching a patch that contains the the constructors as well as the
>>>> NumericTraits for DiffusionTensor3D pixeltypes.
>>>>
>>>> Please let me know if the added tests are sufficient.
>>>>
>>>> I'm running an Experimental build now If it is green should i commit the
>>>> the
>>>> changes?
>>>>
>>>> -Luke
>>>>
>>>> Luis Ibanez wrote:
>>>>
>>>>
>>>>> Hi Karthik,
>>>>>
>>>>> Thanks for pointing this out.
>>>>>
>>>>> You are right,
>>>>> the FixedArray already provide a templated implementation
>>>>> of the operator=() as well as the casting constructor.
>>>>>
>>>>>
>>>>>
>>>>> Luke:
>>>>>
>>>>> Would you like to give it a shot at implementing these
>>>>> methods in the itkSymmetricSecondRankTensor ?
>>>>>
>>>>> You may want to follow the example of
>>>>>
>>>>> itkFixedArray.h lines:
>>>>>
>>>>> 145-155 : Constructor with casting
>>>>>
>>>>> 171-183 : operator= with casting
>>>>>
>>>>>
>>>>> The corresponding tests for these methods should
>>>>> be added to the file:
>>>>>
>>>>> Insight/Testing/Code/Common/
>>>>> itkSymmetricSecondRankTensorTest.cxx
>>>>>
>>>>>
>>>>> Please let us know if you run into any problem.
>>>>>
>>>>>
>>>>> It will be nice if you could start trying this code soon,
>>>>> as we may move into cutting the ITK 3.18 release
>>>>> as soon as we sort out the licensing issues of the
>>>>> numerical libraries.
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>> Luis
>>>>>
>>>>>
>>>>>
>>>>> --------------------------------------
>>>>> On Tue, Mar 16, 2010 at 2:26 AM, Karthik Krishnan
>>>>> <karthik.krishnan at kitware.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> For FixedArray, Vector, RGBPixel classes, we've already templated the
>>>>>> operator= and constructor classes to provide implicit casting
>>>>>> operations and they seem to work well.
>>>>>>
>>>>>> Thanks
>>>>>> --
>>>>>> karthik
>>>>>>
>>>>>> On Mon, Mar 15, 2010 at 4:43 PM, Luis Ibanez <luis.ibanez at kitware.com>
>>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Hi Brad,
>>>>>>>
>>>>>>> I can't think of a drawback for the explicit constructors
>>>>>>> right now,... (but I may just be low in coffee). :-/
>>>>>>>
>>>>>>>
>>>>>>> I worry about the "invisibility" of these methods, and how
>>>>>>> easy it is to add them, and how hard it will be to remove
>>>>>>> them if something goes wrong with them.
>>>>>>>
>>>>>>>
>>>>>>> I still would vote for the "CastFrom()" methods, but if
>>>>>>> enough developers are happy with explicit constructors
>>>>>>> and are willing to help with the maintenance of the code
>>>>>>> for the next decade, then I'm happy to follow the crowd. :-)
>>>>>>>
>>>>>>>
>>>>>>> Luis
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -----------------------------------------------------------------------------
>>>>>>> On Mon, Mar 15, 2010 at 4:03 PM, Bradley Lowekamp
>>>>>>> <blowekamp at mail.nih.gov> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Hello Luis,
>>>>>>>>
>>>>>>>> Thanks for the info regarding the itk::Array, I forgot the usefulness
>>>>>>>> of all the inherited methods!
>>>>>>>>
>>>>>>>>
>>>>>>>> In regards to this, what is the down side to using an explicit
>>>>>>>> constructor? For example:
>>>>>>>>
>>>>>>>> explicit
>>>>>>>> template<TOtherValueType>
>>>>>>>> SymmetricSecondRankTensor( const
>>>>>>>> SymmetricSecondRankTensor<TOtherValueType> &);
>>>>>>>>
>>>>>>>> This uses standard conventions along with requiring the
>>>>>>>> contractor/conversion to be explicitly defined?
>>>>>>>>
>>>>>>>> Making the following valid code:
>>>>>>>>
>>>>>>>> Float3DTensorType floatTensor2(intTensor);
>>>>>>>> floatTensor2 = Float3DTensorType(intTensor);
>>>>>>>>
>>>>>>>> But not:
>>>>>>>> floatTensor2 = intTensor;
>>>>>>>>
>>>>>>>> Just a thought on an alternative approach....
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Brad
>>>>>>>>
>>>>>>>> p.s.
>>>>>>>> This is my understanding of C++, compilers and standards may differ
>>>>>>>> :)
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mar 15, 2010, at 3:45 PM, Luis Ibanez wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> Hi Luke,
>>>>>>>>>
>>>>>>>>> Historically we have been cautious when adding implicit
>>>>>>>>> conversions, since once that syntactic sugar is introduced
>>>>>>>>> in the code, it is very easy to get unintended consequences
>>>>>>>>> particularly when using templated code.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> For the case of the itkPoint we were a lot more explicit, and
>>>>>>>>> added a method:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> itk::Point<T>::CastFrom( const itk::Point< R > & otherPoint );
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> See the definition in:
>>>>>>>>>
>>>>>>>>> Insight/Code/Common/itkPoint.h
>>>>>>>>>
>>>>>>>>> in lines 212-221:
>>>>>>>>>
>>>>>>>>> /** Copy from another Point with a different representation type.
>>>>>>>>> * Casting is done with C-Like rules */
>>>>>>>>> template < typename TCoordRepB >
>>>>>>>>> void CastFrom( const Point<TCoordRepB,NPointDimension> & pa )
>>>>>>>>> {
>>>>>>>>> for(unsigned int i=0; i<NPointDimension; i++ )
>>>>>>>>> {
>>>>>>>>> (*this)[i] = static_cast<TCoordRep>( pa[i] );
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I would suggest that we follow this Explicit approach for
>>>>>>>>> the Tensor classes.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Would you like to give it a try at implementing and testing
>>>>>>>>> the CastFrom() methods for these classes ?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Luis
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ----------------------------------------------------------------------------------
>>>>>>>>> On Fri, Mar 5, 2010 at 2:00 PM, Luke Bloy <luke.bloy at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> The classes itkDiffusionTensor3D and itkSymmetricSecondRankTensor
>>>>>>>>>> don't have
>>>>>>>>>> constructors that enable casting. (
>>>>>>>>>> http://www.itk.org/Bug/view.php?id=10323
>>>>>>>>>> ) so code such as the following fails.
>>>>>>>>>>
>>>>>>>>>> typedef itk::SymmetricSecondRankTensor<int,3> Int3DTensorType;
>>>>>>>>>> typedef itk::SymmetricSecondRankTensor<float,3>
>>>>>>>>>> Float3DTensorType;
>>>>>>>>>> typedef itk::SymmetricSecondRankTensor<double,3>
>>>>>>>>>> Double3DTensorType;
>>>>>>>>>> Int3DTensorType intTensor(1);
>>>>>>>>>>
>>>>>>>>>> //Test constructors
>>>>>>>>>> Float3DTensorType floatTensor(intTensor);
>>>>>>>>>> Double3DTensorType doubleTensor(floatTensor);
>>>>>>>>>>
>>>>>>>>>> //test Assignment
>>>>>>>>>> Float3DTensorType floatTensor2 = intTensor;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> adding templated constructors and assignment operators will allow
>>>>>>>>>> this type
>>>>>>>>>> of behavior.
>>>>>>>>>>
>>>>>>>>>> Is there a reason these constructors have been not been defined?
>>>>>>>>>>
>>>>>>>>>> -Luke
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> 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
>>>>>
>>>>>
>>>>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itkDti_vs6.patch
Type: text/x-patch
Size: 3815 bytes
Desc: not available
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20100316/3e1c56e2/attachment-0001.bin>
More information about the Insight-developers
mailing list