[Minuit2] Fix analytical Hessian/G2 transform for parameters with limits#22700
Conversation
When a parameter has limits, Minuit2 minimizes in an internal coordinate q that is a non-linear function of the external parameter. Converting a user-provided *external* Hessian (or G2) to internal coordinates therefore requires, on the diagonal, an extra term involving the second derivative of the transformation: ``` H_int(i,i) = (dx/dq)^2 * H_ext(i,i) + (d^2x/dq^2) * g_ext(i) ``` `AnalyticalGradientCalculator::Hessian()` and `::G2()` only applied the first-order Jacobian factor `(dx/dq)^2` and dropped the `(d^2x/dq^2) * g_ext(i)` term. The numerical path does not have this problem because it differentiates directly in internal coordinates, so the term is included implicitly. This was introduced in 888a767 ("[math][minuit2] First implementation in Minuit2 of mNHesse using external Hessian calculator"), which added the external->internal Hessian/G2 transform. At the minimum the external gradient is ~0, so the missing term vanishes and the final errors are correct. But it is non-zero everywhere else, in particular at the seeding point. When a parameter starts close to a limit (dx/dq small) and far from the solution (g_ext large), the dropped term dominates: the seed G2/covariance built by MnSeedGenerator is corrupted and Migrad can take a catastrophic first step, silently converging to a wrong result that is still reported as valid. Relying on the numerical Hessian for the same fit works fine. Fix this by adding the missing curvature term: * add `D2Int2Ext()` to Sin/SqrtLow/SqrtUp parameter transformations and a dispatching `MnUserTransformation::D2Int2Ext()` (returns 0 without limits, so unlimited fits are unchanged) * add `(d^2x/dq^2) * g_ext(i)` to the diagonal in `AnalyticalGradientCalculator::Hessian()` and both branches of `::G2()` The off-diagonal entries are unchanged: the transformation is per-parameter (diagonal), so mixed second derivatives only get the `(dx/dq_i)(dx/dq_j)` factor. Add a regression test (Minuit2.AnalyticalHessianLimitTransformation). It does not rely on the end-to-end Migrad convergence behaviour, which is chaotic and compiler/optimization dependent for the reproducer, but instead checks the analytical internal Hessian/G2 against a central finite difference of the internal gradient at a near-limit, non-minimum point. Without the fix the G2 value is off by several orders of magnitude and has the wrong sign. Closes root-project#22692. 🤖 Done with the help of AI.
Test Results 22 files 22 suites 2d 11h 32m 35s ⏱️ For more details on these failures, see this check. Results for commit dfa5a9e. ♻️ This comment has been updated with latest results. |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM!
Thank you Jonas for finding and fixing this important issue.
I have now a doubt about the Hessian matrix transformation (int to ext and viceversa) that is implemented in MnUserTransformation::Int2extCovariance and Ext2IntCovariance. Should we also include the second derivatives of the transformation for the diagonal term?
There is no need for the off-diagonal since the Hessian of the transformation is diagonal.
|
Good question! I'll investigate. |
|
/backport to 6.40 |
|
Preparing to backport PR #22700 to branch 6.40 requested by guitargeek |
|
Something went wrong when assigning the PR or setting labels @guitargeek please see the logs |
|
This PR has been backported to branch 6.40: #22713 |
|
Thanks for the quick fix! |
|
Yes, that's the plan. I just think it can't be done by the bot because of diverging history, so I wanted to do in manually later. But let's try |
|
/backport to 6.36 |
|
Preparing to backport PR #22700 to branch 6.36 requested by guitargeek |
|
Something went wrong with the creation of the PR to backport to 6.36: @guitargeek please see the logs |
|
This PR has been backported to |
5 participants
When a parameter has limits, Minuit2 minimizes in an internal coordinate q that is a non-linear function of the external parameter. Converting a user-provided external Hessian (or G2) to internal coordinates therefore requires, on the diagonal, an extra term involving the second derivative of the transformation:
AnalyticalGradientCalculator::Hessian()and::G2()only applied the first-order Jacobian factor(dx/dq)^2and dropped the(d^2x/dq^2) * g_ext(i)term. The numerical path does not have this problem because it differentiates directly in internal coordinates, so the term is included implicitly.This was introduced in 888a767 ("[math][minuit2] First implementation in Minuit2 of mNHesse using external Hessian calculator"), which added the external->internal Hessian/G2 transform.
At the minimum the external gradient is ~0, so the missing term vanishes and the final errors are correct. But it is non-zero everywhere else, in particular at the seeding point. When a parameter starts close to a limit (dx/dq small) and far from the solution (g_ext large), the dropped term dominates: the seed G2/covariance built by MnSeedGenerator is corrupted and Migrad can take a catastrophic first step, silently converging to a wrong result that is still reported as valid. Relying on the numerical Hessian for the same fit works fine.
Fix this by adding the missing curvature term:
D2Int2Ext()to Sin/SqrtLow/SqrtUp parameter transformations and a dispatchingMnUserTransformation::D2Int2Ext()(returns 0 without limits, so unlimited fits are unchanged)(d^2x/dq^2) * g_ext(i)to the diagonal inAnalyticalGradientCalculator::Hessian()and both branches of::G2()The off-diagonal entries are unchanged: the transformation is per-parameter (diagonal), so mixed second derivatives only get the
(dx/dq_i)(dx/dq_j)factor.Add a regression test (Minuit2.AnalyticalHessianLimitTransformation). It does not rely on the end-to-end Migrad convergence behaviour, which is chaotic and compiler/optimization dependent for the reproducer, but instead checks the analytical internal Hessian/G2 against a central finite difference of the internal gradient at a near-limit, non-minimum point. Without the fix the G2 value is off by several orders of magnitude and has the wrong sign.
Closes #22692.
🤖 Done with the help of AI.