Open tianluyuan opened 2 months ago
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.
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.
scikit-hep/iminuit#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?