Skip to content

BUG: Detect non-positive-definite covariance in GaussianMembershipFunction#6037

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:fix-gaussian-det-check
Apr 14, 2026
Merged

BUG: Detect non-positive-definite covariance in GaussianMembershipFunction#6037
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:fix-gaussian-det-check

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Summary

Fixes #3589.

GaussianMembershipFunction::SetCovariance() previously checked

const vnl_matrix_inverse<double> inv_cov(m_Covariance.GetVnlMatrix());
const double det = inv_cov.determinant_magnitude();
if (det < 0.) { itkExceptionStringMacro(\"det( m_Covariance ) < 0\"); }

vnl_matrix_inverse::determinant_magnitude() returns the product of singular values, which is always non-negative, so the det < 0. check is unreachable dead code. A singular or negative-definite covariance matrix would silently slip through and propagate NaNs into the multivariate Gaussian normalization constant downstream (m_PreFactor = 1.0 / (std::sqrt(det) * ...)).

This PR replaces the check with vnl_determinant() on the original (non-inverted) covariance and rejects det <= 0, which catches both singular (det == 0) and non-positive-definite (det < 0) cases. The cost is O(n^3) but n is the measurement-vector dimension (typically 1..16), negligible compared to the SVD already computed for the inverse on the next line.

Diff

+#include \"vnl/algo/vnl_determinant.h\"
 ...
-  // the determinant is then costless this way
-  const double det = inv_cov.determinant_magnitude();
-
-  if (det < 0.)
-  {
-    itkExceptionStringMacro(\"det( m_Covariance ) < 0\");
-  }
+  // Compute the *signed* determinant of the covariance matrix.  See
+  // commit message for the rationale (determinant_magnitude is the
+  // product of singular values and is never negative).
+  const double det = vnl_determinant(m_Covariance.GetVnlMatrix());
+
+  if (det <= 0.0)
+  {
+    itkExceptionMacro(\"Covariance matrix must be positive definite (det = \" << det << \").\");
+  }

Tests

Three new regression cases added to itkGaussianMembershipFunctionTest.cxx:

  1. Singular covariance [[1,1],[1,1]] (det = 0) — must throw.
  2. Negative-definite (indefinite) covariance [[1,2],[2,1]] (det = −3) — must throw.
  3. Well-formed covariance [[2,0.5],[0.5,2]] (det = 3.75, symmetric positive-definite) — must succeed.

AI Assistance

Generated by Claude Code as part of the 2026-04-10 open-issue triage cleanup.

Test plan

  • Local build (ITKStatisticsTestDriver) compiles cleanly
  • itkGaussianMembershipFunctionTest passes locally with the new singular / negative-definite / positive-definite assertions
  • pre-commit run clean (clang-format, kw-pre-commit)
  • CI: Linux, Windows, macOS, Python, ARM

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Numerics Issues affecting the Numerics module labels Apr 10, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review April 12, 2026 02:52
@hjmjohnson
Copy link
Copy Markdown
Member Author

@greptileai review this draft before I make it official

@greptile-apps

This comment was marked as resolved.

Comment thread Modules/Numerics/Statistics/test/itkGaussianMembershipFunctionTest.cxx Outdated
…ction

GaussianMembershipFunction::SetCovariance() previously checked
inv_cov.determinant_magnitude() < 0, which is unreachable because
determinant_magnitude() returns the product of singular values and is
always non-negative.  As a result a singular or negative-definite
covariance matrix would silently slip through and propagate NaNs into
the multivariate Gaussian normalization constant downstream.

Replace with vnl_determinant() on the original (non-inverted) covariance
and reject det <= 0, catching both singular and non-positive-definite
cases.  Extend itkGaussianMembershipFunctionTest with three regression
cases (singular, negative-definite, positive-definite happy path).

Closes InsightSoftwareConsortium#3589
@hjmjohnson hjmjohnson force-pushed the fix-gaussian-det-check branch from 5ac2f5b to d11739f Compare April 12, 2026 08:48
@hjmjohnson hjmjohnson requested a review from dzenanz April 13, 2026 22:14
@hjmjohnson hjmjohnson enabled auto-merge April 14, 2026 01:49
@hjmjohnson hjmjohnson merged commit 27d2536 into InsightSoftwareConsortium:main Apr 14, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Numerics Issues affecting the Numerics module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

itk::GaussianMembershipFunction not triggering exception on negative determinant values

2 participants