`itk::GaussianMembershipFunction` not triggering exception on negative determinant values

[jhlegarreta]

Description

The condition

double det = inv_cov.determinant_magnitude();

if (det < 0.)
{
  itkExceptionMacro(<< "det( m_Covariance ) < 0");
}

in itkGaussianMembershipFunction.hxx:116 apparently tries to tell whether the determinant of the covariance is negative. However, when a test was added to verify this condition in PR #3581, the test failed. Having a closer look, if the VNL method determinant_magnitude computes the magnitude of the determinant, and not the determinant, the value will always be positive, and the condition will never be true. However, the documentation of the VNL method says that it computes the determinant; also if that is true, if the covariance matrix is a negative scalar (e.g. -2), the determinant should be negative, and it is always non-negative.

Thus:

• The documentation of the VNL method is not accurate.
• The itkGaussianMembershipFunction.hxx:116 should compute the determinant, and not its magnitude (if the computations below do require a non-negative value, at least)

Steps to Reproduce

1. Get ITK
2. Configure and build ITK
3. Add a test to the itkGaussianMembershipFunctionTest.cxx file, after line 52, e.g. for MeasurementVectorSize = 1

// Test negative covariance matrix determinant exception
covariance.SetSize(MeasurementVectorSize, MeasurementVectorSize);
covariance.Fill(-1);
ITK_TRY_EXPECT_EXCEPTION(function->SetCovariance(covariance));

4. Execute the test and check that it does not trigger the expected exception.

Expected behavior

The condition is useful and an exception is triggered for negative values, or else, the condition is not necessary.

Actual behavior

The expected exception for covariance matrices with negative determinant values is not triggered because it is its magnitude that it is being tested.

Reproducibility

100%.

Versions

master.

Environment

All platforms failed when the test was added.

Additional Information

Did not study what the determinant_magnitude computes in reality.

[Leengit]

if the covariance matrix is a negative scalar (e.g. -2), the determinant should be negative, and it is always non-negative.

I'm not sure what is meant by a "covariance matrix is a negative scalar," so just in case, I'll point out that a two-by-two diagonal matrix with -2 for each diagonal entry has a positive determinant, 4.

[jhlegarreta]

I'm not sure what is meant by a "covariance matrix is a negative scalar,"

A 1x1 matrix having a negative value. Weird dimensionality, I know, but easy to test. Even more so if you consider that the test uses such dimensionality.

I'll point out that a two-by-two diagonal matrix with -2 for each diagonal entry has a positive determinant, 4.

Lee, of course, I know that.

I invite you to try it yourself.

[dzenanz]

The code was introduced by 63dcc47e9b5a335d455cb4bf15a2a94c96c8ef1e, 12 years ago. @hjmjohnson did definition of determinant_magnitude in VNL change since then?

[hjmjohnson]

@dzenanz That does not ring a bell with me. I won't be able to dig through vnl for a few days.

[issakomi]

It is in fact difficult, looks like the determinant_magnitude() is always positive, and must be always positive, the return value is singval_t type and it is abs_t type (for T double is the same double, for T int it is unsigned int, etc, BTW).

The matrix

-1 0 
0 1 

is decomposed into U, W, V matrices

vnl_svd<T>:
U = [
-1 0 
0 1 
]
W = diag([ 1 1 ])
V = [
1 0 
0 1 
]

with the result (product of W diagonal values) is 1.

Just FYI, i don't have a suggestion in particular case.

[jhlegarreta]

Thanks for the investigation @issakomi.

looks like the determinant_magnitude() is always positive, and must be always positive, the return value is singval_t type and it is abs_t type (for T double is the same double, for T int it is unsigned int, etc, BTW).

Then,

• The method documentation/comment must be wrong.
• The ITK code at issue should not call this method, and should instead call the appropriate method to compute the determinant.

I have not investigated why a positive determinant would be required for the ITK method at isue.

[issakomi]

why a positive determinant would be required

that det is used in std::sqrt(det), i guess it is the reason, probably the check det < 0 is obsolete, it would be a bug if determinant_magnitude() returned negative double value, IMHO, probably the block with exception is unreachable for coverage. Maybe VNL experts could clarify.

[jhlegarreta]

that det is used in std::sqrt(det),

Of note, the variable det is also used in a condition previous to that statement: https://github.com/InsightSoftwareConsortium/ITK/blob/05adf2d1f617c2c4b206cb119f72cbb0904158e4/Modules/Numerics/Statistics/include/itkGaussianMembershipFunction.hxx#L125

[issakomi]

the variable det is also used in a condition previous

Oh, yes, you are right.