[v636][Minuit2] Fix analytical Hessian/G2 transform for parameters with limits#22714
Open
guitargeek wants to merge 1 commit into
Open
[v636][Minuit2] Fix analytical Hessian/G2 transform for parameters with limits#22714guitargeek wants to merge 1 commit into
guitargeek wants to merge 1 commit into
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. (cherry picked from commit 5db334e)
Test Results 16 files 16 suites 2d 12h 41m 6s ⏱️ For more details on these failures, see this check. Results for commit a022d11. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of:
#22700