[Insight-developers] SymmetricSecondRankTensor does incorrect matrix PreMultiply and PostMultiply

[Bradley Lowekamp]

Kirs,

How common this the usage: A = g*T*g'?

If the explicit keyword is used for a copy constructor it make it difficult to accidentally convert to the type under normal circumstances. However, will ITK's exuberant usage of static_casts when there is a conversion warning, it may unintentionally happen in some places in ITK. Though looking at the class it already has several constructors which may be unused implicitly, so If you make an explicit constructor which takes the Matrix type as an argument I think it would be OK. 


Alternatively if you are frequently using the above equation, why don't you implement it as a method. There is nothing fancy done in ITK matrix math, just loops. You should be able to quite easily implement a specialized method for this case.

It's not clear to me how you are intending to use the SymmetricSecondRankTensor class. Did you consider the vnl_sym_matrix class? It is my understanding that the SymmetricSecondRankTensor class was designed for a fairly specific purpose, so if it's not meeting your needs there may be alternative. Additionally, it has a constructor similar to what you may need for itk's version:

  //: Construct a symmetric matrix from a full matrix.
  // If NDEBUG is set, the symmetry of the matrix will be asserted.
  inline explicit vnl_sym_matrix(vnl_matrix<T> const& that);


So it may be good for reference too.

Brad

On May 29, 2012, at 9:43 AM, Kris Zygmunt wrote:

> The fix for this bug is to return a Matrix instead of a SymmetricSecondRankTensor, but if you have some math like: 
> 
> A = g*T*g' where T is a tensor and you know that the result A is a tensor, it would be nice to have a way to convert the Matrix that is produced by T.PreMultiply(g).PostMultiply(g.GetTranspose()) into a Tensor.  A copy constructor or assignment operator may not be ideal as they could lead to unintended consequences from accidentally converting from a Matrix to a Tensor, but perhaps there could be a method with a name like InitFromMatrix so that one could write:
> 
> DiffusionTensor3D newT;
> newT.InitFromMatrix(A);
> 
> I can add this new method under the same gerrit patch  http://review.source.kitware.com/5943 .  Is there any preference for the name?  Any reason this shouldn't be done?
> -Kris
> 
> 
>> Hello,
>>     While working on some code that does math with DiffusionTensor3D  
>> pixels, I found that DiffusionTensor3D's parent class  
>> SymmetricSecondRankTensor returns a SymmetricSecondRankTensor from  
>> both the PreMultiply(MatrixType m) and the PostMultiply(MatrixType  
>> m).  This code is in effect saying that a symmetric tensor multiplied  
>> by any matrix results in a symmetric tensor which is not true!  The  
>> code needs to be changed to return a Matrix instead of a  
>> SymmetricSecondRankTensor.  Anyone who is using these PreMultiply and  
>> PostMultiply methods currently is having important information from  
>> the lower half of the matrix thrown away and getting incorrect  
>> computations as a result.
>> 
>> I have added a patch that fixes this bug at http://review.source.kitware.com/5943
>> 
>> -Kris
> _______________________________________________
> 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.php
> 
> 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

========================================================
Bradley Lowekamp  
Medical Science and Computing for
Office of High Performance Computing and Communications
National Library of Medicine 
blowekamp at mail.nih.gov



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/pipermail/insight-developers/attachments/20120529/a93bc870/attachment.htm>