MantisBT - ITK
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0007327ITKpublic2008-07-11 08:492009-03-10 20:03
ReporterHans Johnson 
Assigned ToTom Vercauteren 
PriorityhighSeverityminorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in VersionITK-3-10 
Resolution Date
Sprint
Sprint Status
Summary0007327: DeformationFieldJacobianDeterminantFilter Produces wrong results
DescriptionThe DeformationFieldJacobianDeterminantFilter, and the comments in the header are misleading as to what the filter is supposed to produce.


See comments from attached mail dialog that indicate the problems in more detail.

Steps To Reproduce
Additional InformationTom,

Thanks for your comments. My understanding of a "Deformation Field Jacobian" also makes the default behavior of the current filter meaningless. It is also my opinion that the filter is not computing the correct result, and that the comment in the header of that file is incorrect

< * should be 1.0. In general, in order to obtain the effective deformation
< * Jacobian determinant, 1.0 must be added to each Jacobian value in the
< * output.
---
> * is 1.0. In order to compute the effective deformation Jacobian determinant
> * 1.0 must be added to the diagonal elements of Jacobian prior to taking the derivative.
> * i.e. det([ (1.0+dx/dx) dx/dy dx/dz ; dy/dx (1.0+dy/dy) dy/dz; dz/dx dz/dy (1.0+dz/dz) ])

================================================
I did not want two slightly different versions of the same filter with different template signatures. I also thought that it would be difficult for a new user to determine which one to use because of the coding style and template differences even though the logical differences were quite small.

I very much like the idea of deprecating the current filter (unless someone can indicate that the answers produced are actually correct in some circumstances). This would also free us to change the template parameter signature of the filter without causing end user confusion about two filters that on the surface appear to claim to do the same thing.

Whatever the final decision is, we need to highlight the differences between the two filters and give appropriate use cases for each filter in the header comments of both filters.

================================================
Proposal:

A) Revert DeformationFieldJacobianDeterminantFilter to it's previous state, and mark it as deprecated.
B) Match the template parameters of WarpJacobianDeterminantFilter to that of DeformationFieldJacobianDeterminantFilter so that the internal computations of the jacobian can be done in either float or double. This also allows easy replacement of the DeformationFieldJacobianDeterminantFilter through search and replace.
C) Fast track the inclusion of itkWarpJacobianDeterminant filter out of Review and into ITK. This should be possible because it is a bug fix, and is the most reasonable way to preserve backwards compatibility.

I'll collect comments this weekend, and start working on this solution.

Thanks,
Hans


On 7/11/08 2:47 AM, "Tom Vercauteren" <tom.vercauteren@m4x.org> wrote:

> Hi Hans,
>
> Thanks for the heads up!
>
> I remember discussing the DeformationFieldJacobianDeterminantFilter
> issue some time ago. See the following posts:
> http://www.itk.org/pipermail/insight-users/2007-July/022858.html [^]
> http://www.itk.org/pipermail/insight-users/2007-July/022866.html [^]
>
> Adding a flag to DeformationFieldJacobianDeterminantFilter and
> removing WarpJacobianDeterminantFilter is OK for me. The only thing
> that bugs me is that the default behavior of
> DeformationFieldJacobianDeterminantFilter (i.e. the current one) would
> be to compute a somewhat "meaningless" quantity. And as far as I
> understand it, it might not be possible to set
> m_GenerateEffectiveJacobianDeterminant to true by default since this
> would break backwards compatibility.
>
> However, if we consider the current behavior of
> DeformationFieldJacobianDeterminantFilter to be a bug (which I do),
> changing the default behavior should IMHO not be considered as
> breaking backwards compatibility. Other people might have a different
> feeling about that though:
> http://www.vtk.org/Wiki/ITK_Backward_Compatibility_Open_Discussion [^]
>
> Another option might be to;
> 1) Allow both behaviors in WarpJacobianDeterminantFilter
> 2) Set the default behavior of WarpJacobianDeterminantFilter to be
> m_GenerateEffectiveJacobianDeterminant=true
> 3) Put WarpJacobianDeterminantFilter in the main ITK tree (not in Review)
> 4) Deprecate DeformationFieldJacobianDeterminantFilter
>
> The advantage of this would also be that the name of the filter
> WarpJacobianDeterminantFilter would better represent the default
> behavior.
>
> Regards,
>
> Tom
>
> On Fri, Jul 11, 2008 at 3:29 AM, Johnson, Hans <hans-johnson@uiowa.edu> wrote:
>> Luis,
>>
>> I see that you are adding the Diffeomorphic Demons to ITK Review. GREAT!
>> I've been working with that code for a few months now, and it really is a
>> nice addition to ITK.
>>
>> I was reviewing the code in itkWarpJacobianDeterminantFilter.h, and
>> comparing it against itkDeformationFieldJacobianDeterminantFilter.h It
>> seems that all the functionality of the WarpJacobianDeterminant can be added
>> to the DeformationFieldJacobianDeterminant with little effort. I've
>> attached my local copies that have the necessary changes (it passes
>> regression test for being backwards compatible, and it address the comment
>> in itkDeformationFieldJacobianDeterminantFilter.
>>
>> NOTE 1: itkWarpJacobianDeterminant and
>> itkDeformationFieldJacobianDeterminant have different template
>> parameterization, but this seems to be a style choice.
>> NOTE 2: I've added another private member variable to precompute the
>> 0.5*weightings. This is not a necessary change for getting the new
>> functionality.
>>
>> I'd like to know your thoughts if this seems like a reasonable change. If
>> you agree, I will work through committing the enhanced version of
>> itkDeformationFieldJacobianDeterminant, and removing the
>> itkWarpJacobianDeterminantFilter from itk Review.
>>
>> Thanks,
>> Hans
>>
>> ===== Patch ===
>> Index: itkDeformationFieldJacobianDeterminantFilter.h
>> ===================================================================
>> RCS file:
>> /cvsroot/Insight/Insight/Code/BasicFilters/itkDeformationFieldJacobianDetermi
>> nantFilter.h,v
>> retrieving revision 1.3
>> diff -b -r1.3 itkDeformationFieldJacobianDeterminantFilter.h
>> 6c6
>> < Date: $Date: 2005-08-24 18:00:04 $
>> ---
>>> Date: $Date: 2005/08/24 18:00:04 $
>> 42,44c42,44
>> < * should be 1.0. In general, in order to obtain the effective deformation
>> < * Jacobian determinant, 1.0 must be added to each Jacobian value in the
>> < * output.
>> ---
>>> * is 1.0. In order to compute the effective deformation Jacobian
>>> determinant
>>> * 1.0 must be added to the diagonal elements of Jacobian prior to taking
>>> the derivative.
>>> * i.e. det([ (1.0+dx/dx) dx/dy dx/dz ; dy/dx (1.0+dy/dy) dy/dz; dz/dx
>>> dz/dy (1.0+dz/dz) ])
>> 240d239
>> < unsigned i, j;
>> 243c242
>> < for (i = 0; i < ImageDimension; ++i)
>> ---
>>> for (unsigned int i = 0; i < ImageDimension; ++i)
>> 245c244
>> < for (j = 0; j < VectorDimension; ++j)
>> ---
>>> for (unsigned int j = 0; j < VectorDimension; ++j)
>> 247,248c246
>> < J[i][j] = m_DerivativeWeights[i]
>> < * 0.5 * (it.GetNext(i)[j] - it.GetPrevious(i)[j]);
>> ---
>>> J[i][j] = m_HalfDerivativeWeights[i] * (it.GetNext(i)[j] -
>>> it.GetPrevious(i)[j]);
>> 249a248,251
>>> if(m_GenerateEffectiveJacobianDeterminant)
>>> {
>>> // add one on the diagonal to consider the warping and not only
>>> the deformation field
>>> J[i][i] += 1.0;
>> 251,252c253,255
>> <
>> < return vnl_det(J);
>> ---
>>> }
>>> const TRealType value=vnl_det(J);
>>> return value;
>> 256a260,261
>>> /** Pre-compute 0.5*m_DerivativeWeights since that is the only thing
>>> used in the computations. */
>>> TRealType
>>> m_HalfDerivativeWeights[itk::GetImageDimension<TInputImage>::ImageDimension]>>> ;
>> 267a273
>>> bool m_GenerateEffectiveJacobianDeterminant;
>> Index: itkDeformationFieldJacobianDeterminantFilter.txx
>> ===================================================================
>> RCS file:
>> /cvsroot/Insight/Insight/Code/BasicFilters/itkDeformationFieldJacobianDetermi
>> nantFilter.txx,v
>> retrieving revision 1.3
>> diff -b -r1.3 itkDeformationFieldJacobianDeterminantFilter.txx
>> 6c6
>> < Date: $Date: 2006-01-11 19:43:31 $
>> ---
>>> Date: $Date: 2006/01/11 19:43:31 $
>> 37d36
>> < unsigned int i;
>> 40c39
>> < for (i = 0; i < ImageDimension; i++)
>> ---
>>> for (unsigned int i = 0; i < ImageDimension; i++)
>> 43a43
>>> m_HalfDerivativeWeights[i] = static_cast<TRealType>(0.5);
>> 44a45
>>> m_GenerateEffectiveJacobianDeterminant=false;//Needs to be false for
>>> backwards compatibility;
>> 58a60
>>> m_HalfDerivativeWeights[i] = 0.5*data[i];
>> 79a82
>>> m_HalfDerivativeWeights[i] = static_cast<TRealType>(0.5);
>> 156a160
>>> m_HalfDerivativeWeights[i]=0.5*m_DerivativeWeights[i];
>> 240a245,248
>>> os << indent << "m_HalfDerivativeWeights = ";
>>> for (i = 0; i < ImageDimension; i++)
>>> { os << m_HalfDerivativeWeights[i] << " "; }
>>> os << std::endl;
>> 244a253,254
>>> os << indent << "m_GenerateEffectiveJacobianDeterminant = " <<
>>> m_GenerateEffectiveJacobianDeterminant
>>> << std::endl;
>> _______________________________________________
>> Insight-developers mailing list
>> Insight-developers@itk.org
>> http://www.itk.org/mailman/listinfo/insight-developers [^]
>>
>>
TagsNo tags attached.
Relationships
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2008-07-11 08:49Hans JohnsonNew Issue
2009-03-05 14:18Tom VercauterenNote Added: 0015572
2009-03-05 14:19Tom VercauterenNote Added: 0015573
2009-03-05 14:19Tom VercauterenStatusnew => resolved
2009-03-05 14:19Tom VercauterenResolutionopen => fixed
2009-03-05 14:19Tom VercauterenAssigned To => Tom Vercauteren
2009-03-10 20:03Hans JohnsonNote Added: 0015632
2009-03-10 20:03Hans JohnsonStatusresolved => closed
2009-03-10 20:03Hans JohnsonFixed in Version => ITK-3-10

Notes
(0015572)
Tom Vercauteren   
2009-03-05 14:18   
A filter called DisplacementFieldJacobianDeterminantFilter that performs the correct computation by default has been implemented:
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkDisplacementFieldJacobianDeterminantFilter.h?root=Insight&sortby=date&view=markup [^]

The documentation of DeformationFieldJacobianDeterminantFilter has been updated to reflect the fact that it produces counter-intuitive results:
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkDeformationFieldJacobianDeterminantFilter.h?root=Insight&r1=1.5&r2=1.3&sortby=date [^]
(0015573)
Tom Vercauteren   
2009-03-05 14:19   
Fixed by committing DisplacementFieldJacobianDeterminantFilter and updating the documentation
(0015632)
Hans Johnson   
2009-03-10 20:03   
Fixed by adding a new class that has the correct behavior. Thanks Tom Vercauteren