Skip to content

Fix missing PSD projection of mollified m≤0 hessian blocks#244

Merged
zfergus merged 5 commits into
mainfrom
fix/mollified-hessian-psd-projection
Jul 18, 2026
Merged

Fix missing PSD projection of mollified m≤0 hessian blocks#244
zfergus merged 5 commits into
mainfrom
fix/mollified-hessian-psd-projection

Conversation

@zfergus

@zfergus zfergus commented Jul 18, 2026

Copy link
Copy Markdown
Member

Problem

NormalPotential::hessian() has an early-return for the mollified case when the mollifier m == 0 (exactly parallel edges):

if (collision.is_mollified() && m <= 0) {
    const double f = (*this)(d, collision.dmin);
    const MatrixMax12d hess_m = collision.mollifier_hessian(positions);
    return (collision.weight * f) * hess_m;   // ← returned WITHOUT PSD projection
}

Every other return path applies project_to_psd, but this one did not. For positive weights the block (weight·f)·hess_m is PSD, so skipping projection was harmless. However, IMPROVED_MAX_APPROX produces negative-weight collisions, which flip the block to negative-(semi)definite — so the assembled "PSD-projected" hessian could contain non-PSD contributions. This was found via a GPU-vs-CPU hessian parity mismatch that reproduces on the cube (axis-aligned edges → m == 0 exactly) with IMPROVED_MAX_APPROX + area weighting + CLAMP/ABS projection.

Fix

Project the block like the other paths. Because m == 0 (⟺ ‖cross‖² == 0) is a global minimum of the mollifier, hess_m = ∇²m is PSD, and f = f(d) > 0. The block is therefore a scalar multiple of a PSD matrix, so its PSD projection reduces to projecting the scalar weight·f:

  • NONEweight·f · hess_m
  • CLAMPmax(weight·f, 0) · hess_m (negative weight → zero)
  • ABS|weight·f| · hess_m

No eigendecomposition is needed, so this is also cheaper than a general projection in the common parallel-edge case. NONE behavior is unchanged, so existing finite-difference gradient/hessian tests are unaffected.

Testing

  • Added a regression test that asserts the assembled CLAMP hessian is PSD for a cube under IMPROVED_MAX_APPROX (exercises the m == 0 branch). It fails before this change and passes after.
  • Full [barrier_potential] suite passes (7418 assertions, 13 cases).

🤖 Generated with Claude Code

NormalPotential::hessian() early-returns the mollified block (weight·f)·∇²m
when the mollifier m == 0 (exactly parallel edges), but did so WITHOUT PSD
projection while every other path projects. For positive weights the block is
PSD so this was harmless, but IMPROVED_MAX_APPROX produces negative-weight
collisions, making the block negative-(semi)definite and the assembled
"PSD-projected" hessian non-PSD.

Project the block like the other paths. Since m == 0 is a global minimum of the
mollifier, ∇²m is PSD and f = f(d) > 0, so the block is a scalar multiple of a
PSD matrix and its projection reduces to projecting the scalar weight·f
(clamp/abs) -- no eigendecomposition needed.

Add a regression test asserting the assembled CLAMP hessian is PSD for a
cube under IMPROVED_MAX_APPROX (which exercises the m == 0 branch).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 18, 2026 18:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a missing PSD-projection in NormalPotential::hessian() for the mollified m <= 0 early-return path (parallel-edge case), which could previously inject non-PSD blocks into the assembled Hessian when using negative-weight collisions (e.g., IMPROVED_MAX_APPROX with CLAMP/ABS projection).

Changes:

  • Update the m <= 0 early-return branch to apply PSD projection by projecting the scalar (weight * f) and scaling hess_m accordingly (avoids eigendecomposition for this case).
  • Add a regression test asserting the assembled Hessian is PSD for a cube under IMPROVED_MAX_APPROX with PSDProjectionMethod::CLAMP.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/ipc/potentials/normal_potential.cpp Applies PSD projection logic to the m <= 0 mollified Hessian early-return block via scalar projection.
tests/src/tests/potential/test_barrier_potential.cpp Adds a regression test to catch non-PSD assembled Hessians caused by the m == 0 early-return path under negative weights.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/src/tests/potential/test_barrier_potential.cpp Outdated
Comment thread tests/src/tests/potential/test_barrier_potential.cpp
Comment thread src/ipc/potentials/normal_potential.cpp Outdated
zfergus and others added 4 commits July 18, 2026 14:32
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Reword the m<=0 comment in ASCII with shorter lines so clang-format 20
  (used in CI) leaves it unchanged.
- test_barrier_potential.cpp: NormalCollisions has no begin()/end(), so the
  range-based for over collisions did not compile; use an index loop over
  operator[]/size(). Also include <Eigen/Eigenvalues> for SelfAdjointEigenSolver
  and use SparseMatrix::toDense() instead of the sparse->dense constructor.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.58%. Comparing base (aa89460) to head (f9e4ef2).

Files with missing lines Patch % Lines
src/ipc/potentials/normal_potential.cpp 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
+ Coverage   96.51%   96.58%   +0.07%     
==========================================
  Files         163      163              
  Lines       16651    16656       +5     
  Branches      920      922       +2     
==========================================
+ Hits        16070    16087      +17     
+ Misses        581      569      -12     
Flag Coverage Δ
unittests 96.58% <83.33%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zfergus
zfergus merged commit 0697169 into main Jul 18, 2026
21 checks passed
@zfergus
zfergus deleted the fix/mollified-hessian-psd-projection branch July 18, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants