Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task done
alexander-held opened this issue Aug 8, 2022 · 9 comments
Open
1 task done
Labels
bug Something isn't working needs-triage Needs a maintainer to categorize and assign

Comments

@alexander-held
Copy link
Member

alexander-held commented Aug 8, 2022

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: scikit-hep/iminuit#746):

message = "Optimization terminated successfully."
if not minimizer.valid:
message = "Optimization failed."

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

  • I agree to follow the Code of Conduct
@alexander-held alexander-held added bug Something isn't working needs-triage Needs a maintainer to categorize and assign labels Aug 8, 2022
@matthewfeickert matthewfeickert moved this to Todo in pyhf v0.7.2 Apr 7, 2023
@matthewfeickert matthewfeickert moved this to Todo in pyhf v0.7.3 May 18, 2023
@matthewfeickert matthewfeickert moved this to Todo in pyhf v0.7.4 Aug 16, 2023
@matthewfeickert matthewfeickert moved this to Todo in pyhf v0.7.5 Sep 6, 2023
@condrine
Copy link

condrine commented Dec 7, 2023

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
Copy link
Member Author

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
Copy link
Contributor

kratsg commented Dec 8, 2023

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
Copy link

condrine commented Dec 8, 2023

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
Copy link
Member

matthewfeickert commented Dec 8, 2023

Related to what @kratsg was saying, looking at

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()

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.

@condrine
Copy link

condrine commented Dec 8, 2023

Related to what @kratsg was saying, looking at

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()

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
Copy link
Member

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
Copy link
Member Author

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

message = "Optimization terminated successfully."
if not minimizer.valid:
message = "Optimization failed."
fmin = minimizer.fmin
if fmin.has_reached_call_limit:
message += " Call limit was reached."
if fmin.is_above_max_edm:
message += " Estimated distance to minimum too large."

below the Hesse section

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()
hess_inv = minimizer.covariance
corr = hess_inv.correlation()
unc = minimizer.errors

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

@condrine
Copy link

condrine commented Dec 8, 2023

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

message = "Optimization terminated successfully."
if not minimizer.valid:
message = "Optimization failed."
fmin = minimizer.fmin
if fmin.has_reached_call_limit:
message += " Call limit was reached."
if fmin.is_above_max_edm:
message += " Estimated distance to minimum too large."

below the Hesse section

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()
hess_inv = minimizer.covariance
corr = hess_inv.correlation()
unc = minimizer.errors

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-triage Needs a maintainer to categorize and assign
Projects
Status: Todo
Development

No branches or pull requests

4 participants