scikit-hep / pyhf

pure-Python HistFactory implementation with tensors and autodiff
https://pyhf.readthedocs.io/
Apache License 2.0
279 stars 83 forks source link

"Optimization terminated successfully." when HESSE fails after MIGRAD #1928

Open alexander-held opened 2 years ago

alexander-held commented 2 years ago

Summary

When HESSE fails after MIGRAD with the minuit backend, the resulting error is accompanied by a misleading "Optimization terminated successfully." message. This happens since the status is queried before HESSE is run (which can turn a valid into an invalid minimum, related: https://github.com/scikit-hep/iminuit/issues/746): https://github.com/scikit-hep/pyhf/blob/2cd8c0be20b6004a3fdb5305afd13d970589f547/src/pyhf/optimize/opt_minuit.py#L123-L125

OS / Environment

n/a

Steps to Reproduce

This uses a workspace from https://www.hepdata.net/record/resource/2258588?landing_page=True.

curl -OJLH "Accept: application/x-tar" https://doi.org/10.17182/hepdata.100351.v2/r1
tar -xvf likelihood_ttbarZ_13TeV.tar.xz
pyhf fit likelihood_ttbarZ_13TeV/3L_workspace.json --optimizer minuit

File Upload (optional)

No response

Expected Results

The error message should reflect that HESSE failed.

Actual Results

File "[...]/pyhf/src/pyhf/cli/infer.py", line 113, in fit
    fit_result = mle.fit(data, model, return_fitted_val=value)
  File "[...]/pyhf/src/pyhf/infer/mle.py", line 131, in fit
    return opt.minimize(
  File "[...]/pyhf/src/pyhf/optimize/mixins.py", line 185, in minimize
    result = self._internal_minimize(
  File "[...]/pyhf/src/pyhf/optimize/mixins.py", line 65, in _internal_minimize
    raise exceptions.FailedMinimization(result)
pyhf.exceptions.FailedMinimization: Optimization terminated successfully.

pyhf Version

pyhf, version 0.7.0rc2.dev3

Code of Conduct

condrine commented 9 months ago

Hi @alexander-held! Would something like this work here?

try:
  minimizer.hesse()
  hess_inv = minimizer.covariance
  corr = hess_inv.correlation()
  unc = minimizer.errors
  message = "Hesse converges"
except:
  message = "Hesse fails."
alexander-held commented 9 months ago

I assume that would work, but we might need one of the core developers to comment on how exactly the FailedMinimization exception works and where it is raised. It might also be possible to check minimizer.valid again after the minimizer.hesse() call.

kratsg commented 9 months ago

Not sure what you want from the logic. When hesse fails, that's because the minimizer.valid == True right before calling it, so we'd probably want to just update minuit to have the message that Hesse failed.

condrine commented 9 months ago

One thing that could be done is to raise separate exceptions for hesse and migrad, eg FailedMigrad and FailedHesse to differentiate betwen whatever's happening.

matthewfeickert commented 9 months ago

Related to what @kratsg was saying, looking at

https://github.com/scikit-hep/pyhf/blob/1191f6676002b26bbd751c1b8e116400baaed25e/src/pyhf/optimize/opt_minuit.py#L136-L138

this final minimizer.hesse() call can result in minimizer.valid=False. This is again why https://github.com/scikit-hep/iminuit/issues/746 is still open. While we could guess at the cause by doing something like

        if minimizer.valid:
            # Extra call to hesse() after migrad() is always needed for good error estimates.
            # If you pass a user-provided gradient to MINUIT, convergence is faster.
            minimizer.hesse()
            if not minimizer.valid:
                message = "Optimization failed."
                fmin = minimizer.fmin
                if not fmin.has_made_posdef_covar:
                    message += (
                        " Failed to make the covariance matrix positive definite."
                    )

given how HESSE works, it would probably be better to try to go spend time working with Hans to figure out how to deal with https://github.com/scikit-hep/iminuit/issues/746 more appropriately.

condrine commented 9 months ago

Related to what @kratsg was saying, looking at

https://github.com/scikit-hep/pyhf/blob/1191f6676002b26bbd751c1b8e116400baaed25e/src/pyhf/optimize/opt_minuit.py#L136-L138

this final minimizer.hesse() call can result in minimizer.valid=False. This is again why scikit-hep/iminuit#746 is still open. While we could guess at the cause by doing something like

        if minimizer.valid:
            # Extra call to hesse() after migrad() is always needed for good error estimates.
            # If you pass a user-provided gradient to MINUIT, convergence is faster.
            minimizer.hesse()
            if not minimizer.valid:
                message = "Optimization failed."
                fmin = minimizer.fmin
                if not fmin.has_made_posdef_covar:
                    message += (
                        " Failed to make the covariance matrix positive definite."
                    )

given how HESSE works, it would probably be better to try to go spend time working with Hans to figure out how to deal with scikit-hep/iminuit#746 more appropriately.

In this implementation, wouldn't it make more sens to use has_posdef_covar instead of has_made_posdef_covar?

matthewfeickert commented 9 months ago

In this implementation, wouldn't it make more sens to use has_posdef_covar instead of has_made_posdef_covar?

That's probably fine, but my point is more that this is trying to handle something that is out of scope for pyhf and should be handeled in iminuit generally.

alexander-held commented 9 months ago

I think some of this can be done in iminuit, the main thing in my view is not having a "optimization successful" message when the minimum is invalid. Maybe the easiest would be moving this whole block

https://github.com/scikit-hep/pyhf/blob/2cd8c0be20b6004a3fdb5305afd13d970589f547/src/pyhf/optimize/opt_minuit.py#L123-L130

below the Hesse section

https://github.com/scikit-hep/pyhf/blob/2cd8c0be20b6004a3fdb5305afd13d970589f547/src/pyhf/optimize/opt_minuit.py#L135-L140

and (possibly) before Hesse only add something about Migrad to the message?

condrine commented 9 months ago

I think some of this can be done in iminuit, the main thing in my view is not having a "optimization successful" message when the minimum is invalid. Maybe the easiest would be moving this whole block

https://github.com/scikit-hep/pyhf/blob/2cd8c0be20b6004a3fdb5305afd13d970589f547/src/pyhf/optimize/opt_minuit.py#L123-L130

below the Hesse section

https://github.com/scikit-hep/pyhf/blob/2cd8c0be20b6004a3fdb5305afd13d970589f547/src/pyhf/optimize/opt_minuit.py#L135-L140

and (possibly) before Hesse only add something about Migrad to the message?

This is what I was trying to do with my code. However, I am a bit sceptical about the message "Optimisation terminated successfully" in thee first place, as this will probably only appear whenever FailedMinimization exception has been invoked, in which case it'll look misleading.