Fix missing PSD projection of mollified m≤0 hessian blocks#244
Conversation
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>
There was a problem hiding this comment.
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 <= 0early-return branch to apply PSD projection by projecting the scalar(weight * f)and scalinghess_maccordingly (avoids eigendecomposition for this case). - Add a regression test asserting the assembled Hessian is PSD for a cube under
IMPROVED_MAX_APPROXwithPSDProjectionMethod::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.
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 Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
NormalPotential::hessian()has an early-return for the mollified case when the mollifierm == 0(exactly parallel edges):Every other return path applies
project_to_psd, but this one did not. For positive weights the block(weight·f)·hess_mis PSD, so skipping projection was harmless. However,IMPROVED_MAX_APPROXproduces 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 == 0exactly) withIMPROVED_MAX_APPROX+ area weighting +CLAMP/ABSprojection.Fix
Project the block like the other paths. Because
m == 0(⟺‖cross‖² == 0) is a global minimum of the mollifier,hess_m = ∇²mis PSD, andf = f(d) > 0. The block is therefore a scalar multiple of a PSD matrix, so its PSD projection reduces to projecting the scalarweight·f:NONE→weight·f · hess_mCLAMP→max(weight·f, 0) · hess_m(negative weight → zero)ABS→|weight·f| · hess_mNo eigendecomposition is needed, so this is also cheaper than a general projection in the common parallel-edge case.
NONEbehavior is unchanged, so existing finite-difference gradient/hessian tests are unaffected.Testing
CLAMPhessian is PSD for a cube underIMPROVED_MAX_APPROX(exercises them == 0branch). It fails before this change and passes after.[barrier_potential]suite passes (7418 assertions, 13 cases).🤖 Generated with Claude Code