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

Backwards compatibility issue with default use_simplex #1041

Open
tianluyuan opened this issue Sep 21, 2024 · 2 comments
Open

Backwards compatibility issue with default use_simplex #1041

tianluyuan opened this issue Sep 21, 2024 · 2 comments

Comments

@tianluyuan
Copy link

#1009 changes the default migrad routine if iterations are necessary. This may help with convergence issues, but can lead to different results thus breaking backwards compatibility. There may also be cases where this introduces additional redundancy, such as where the user already runs simplex followed by migrad. Would it be possible to default use_simplex=False, at least until the next major version increment?

@tianluyuan tianluyuan changed the title Backwards compatibility issue with use_simplex Backwards compatibility issue with default use_simplex Sep 21, 2024
@HDembinski
Copy link
Member

HDembinski commented Sep 24, 2024

I understand your point, but relying on how iminuit and Minuit2 behave numerically in pathological edge cases is a major issue. There is no guarantee that your edges cases will be solved in the exact same way between versions for a number of reasons. For example, changing the numpy version could in principle change the outcome. If you want full reproducibility, you should pin iminuit and numpy versions, and potentially numba if you use the builtin cost functions.

I do not plan to make another major release for breaking changes anytime soon, so if I follow your suggestion, use_simplex=False would remain for a long time. The defaults are there to make life easier for new users. I assume use_simplex=True works better for these users.

Can you provide evidence that use_simplex=True leads to worse outcomes in your study than use_simplex=False? When you have a lot of fits and some are edge cases, switching this will lead to bad fits in both cases, but not necessarily in the same cases. My goal is to minimize the overall number of failed/bad fits. If you have this evidence, I will set the default to False.

@tianluyuan
Copy link
Author

Indeed, we've pinned numpy<2.0 for now. There are a couple issues that we're seeing in the tests with the updates in 2.27.0. One is that the fits are taking longer, sometimes substantially enough to cause the tests to timeout. The other is of course slight differences in the likelihood, though in cases where the fit completes with iterations the tests do show a slight llh improvement with use_simplex=True. There are also more nan cases, but I have not yet determined if that is due to the iminuit update.

It's a bit complicated to reproduce our setup and fit data, though you can see the sequence of test results in icecube/skymap_scanner#272. Here's a partial output log where one of the fits timed out. Typically this completes in ~6 min.

...
I VariableMetricBuilder    3 - FCN =       2408.685183 Edm =        24.3928393 NCalls =    149
W VariableMetricBuilder Iterations finish without convergence; Edm 24.3928 Requested 0.005
W VariableMetricBuilder FunctionMinimum is invalid after second try
I SimplexBuilder    0 - FCN =       2408.685183 Edm =        3.00801316 NCalls =      5
I SimplexBuilder Final iteration FCN =       2408.685183 Edm =        3.00801316 NCalls =      8
W SimplexBuilder Simplex did not converge, edm > minedm
I MnSeedGenerator Using analytical (external) gradient calculator but cannot compute G2 - use then numerical gradient for G2
I MnSeedGenerator Computing seed using NumericalGradient calculator
I MnSeedGenerator Initial state: FCN =       2408.685183 Edm =      0.1467180714 NCalls =      9
I MnSeedGenerator run Hesse - Initial seeding state: 
  Minimum value : 2408.685183
  Edm           : 24.3928393
  Internal parameters:	[       735189.599      158.9402053          -345.23     -354.3900005]	
  Internal gradient  :	[     -26.79108538     -80.90095199      3097.402837     -269.8293775]	
  Internal covariance matrix:
[[     0.11286994   5.458187e-08 -2.6889324e-06 -4.0883531e-05]
 [   5.458187e-08  1.9574077e-05  3.4518698e-07  4.5553028e-07]
 [ -2.6889324e-06  3.4518698e-07  9.9788755e-07  8.5667077e-07]
 [ -4.0883531e-05  4.5553028e-07  8.5667077e-07   0.0001179197]]]
I VariableMetricBuilder Start iterating until Edm is < 0.005 with call limit = 1000
I VariableMetricBuilder    0 - FCN =       2408.685183 Edm =        24.3928393 NCalls =     70
W VariableMetricBuilder No improvement in line search
I VariableMetricBuilder    1 - FCN =       2408.685183 Edm =        24.3928393 NCalls =     79
Error: The action 'run' has timed out after 15 minutes.

Note that this is for performing a likelihood scan where each fit is a constrained minimization where a subset of the parameters (direction) are fixed. In many cases the constraint can cause the llh to be far from the global minimum and this can pose challenges for the minimizer in terms of convergence rate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants