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

Step size of fixed parameters changes after v2.12.1 due to heuristic being applied #762

Closed
alexander-held opened this issue Jul 17, 2022 · 11 comments

Comments

@alexander-held
Copy link
Member

Hi, the step size guess heuristic in #759 is being applied to fixed parameters. This results in the error of fixed parameters changing during MIGRAD. Previously the step size was carried through without changes. This allowed for quickly filtering out fixed parameters when looking at best-fit values and errors without having to refer back to a mask specifying which parameters are fixed.

Example setup:

from iminuit import Minuit


def f(x):
    return x[0] ** 2 + 0.5 * x[1] ** 2


m = Minuit(f, [1.0, 1.0])
m.fixed = [True, False]
m.errors = [0.0, 0.1]
m.migrad()
print(m.errors.to_dict())

with iminuit==v2.12.1:

{'x0': 0.0, 'x1': 1.4142135622742678}

with iminuit==v2.12.2 (same behavior with v2.12.3.beta2):

[...]/iminuit/util.py:143: UserWarning: Assigned errors must be positive. Non-positive values are replaced by a heuristic.
  warnings.warn(
{'x0': 0.1, 'x1': 1.4142135622742678}```
@HDembinski
Copy link
Member

Assigning zero to errors is invalid, but in the past this was not caught. So in my view it is not a bug, but a feature that this code is now giving you a warning message.

How is this causing problems and why do you set errors to zero in the first place?

@HDembinski
Copy link
Member

Setting the Minuit.errors shouldn't be necessary. C++ Minuit2 largely ignores the initially set values, therefore the heuristic guess set by iminuit should be fine.

@HDembinski
Copy link
Member

@alexander-held Ping

@alexander-held
Copy link
Member Author

What pyhf and cabinetry have done so far is setting Minuit.errors to 0.0 for parameters that were known to be fixed in whatever fit was going to be run next. The .migrad call did not touch .errors. When subsequently listing all post-fit .values and .errors, the values for the parameters that were being held fixed would show up, and their errors would show up as zero. That felt like an intuitive choice to me, as I would get zero standard deviation for a parameter when sampling a model where I hold that parameter constant.

It is straightforward to work around this in code calling iminuit by replacing the .errors for fixed parameters with the desired value after a fit. In some sense we already were working around the behavior before these changes, as not setting .errors initially would have resulted in a non-zero value of .errors for fixed parameters post-fit.

The underlying issue in my view is that .errors has this dual-use of step sizes and parameter errors. That leads to the values having a different meaning post-fit depending on whether or not a given parameter was held constant. Users of iminuit can of course take this into account if they are aware of this behavior, and libraries using iminuit can chose to provide versions of .errors with e.g. values set to 0.0 again after a fit.

Would a "correction" of the final .errors for fixed parameters be something to consider for iminuit, or is this out of scope given that in principle the information can be reconstructed by looking at .fixed?

@HDembinski
Copy link
Member

HDembinski commented Aug 2, 2022

@alexander-held Thank you for the additional discussion.

The underlying issue in my view is that .errors has this dual-use of step sizes and parameter errors. That leads to the values having a different meaning post-fit depending on whether or not a given parameter was held constant. Users of iminuit can of course take this into account if they are aware of this behavior, and libraries using iminuit can chose to provide versions of .errors with e.g. values set to 0.0 again after a fit.

I fully agree with that analysis, but consistently using Minuit.fixed to fix parameters should be the "one obvious way" to do it (quoting zen of python). Fixing parameters by setting errors to zero seems intuitive at first, but I think we cannot rely on C++ Minuit2 code to interpret a step size of zero as a fixed parameter, even if that has worked in the past (?). It is also an issue when you want to release the parameter later again, because then you also need to reset the error/initial step size to a non-zero value. In complicated fits, one often fixes parameters temporarily. Example of signal + background fit:

  1. Mask out the signal region and fix the signal parameters. Fit background parameters.
  2. Fix background parameters and release signal parameters. Unmask the signal region. Fit again.
  3. Release all parameters and fit again.

If iminuit should support fixing parameters by setting errors to zero, we have to agree on a reasonable behavior when the user runs migrad or another minimizer with parameters that have zero step size but are not fixed. In that case, the step size should be set to the heuristic, I think.

@alexander-held
Copy link
Member Author

alexander-held commented Aug 5, 2022

I completely agree with using .fixed as the single way of setting parameters to constant, I don't mean to propose looking up step sizes and fixing parameters if the step size is zero. I have previously also used .fixed (like in the small example above).

The only thing that has changed in my workflow is that previously I set step sizes to zero before fitting (and in addition specified what I'd like to keep fixed via .fixed) and .errors was still zero for these fixed parameters after the fit. This is no longer the case, so now I instead read out .errors into an array and override the values corresponding to fixed parameters with 0.0. I find this slightly less intuitive than before: previously I could fully configure Minuit and return it to the user, who then would automatically get these 0.0 values for fixed parameters when investigating .errors. Now the user has to remember that there might be fixed parameters and interpret .errors accordingly.

I see the complication of repeated fits with new configurations where a zero step size is no longer appropriate. I agree that it makes sense to use the heuristic whenever a non-fixed parameter is encountered with zero step size.

What I would like to suggest considering is not applying the heuristic to parameters that are configured as fixed. The step size for those parameters does not matter for the fit, while in scenarios with multiple fits the heuristic could be applied before the next minimization whenever the .fixed configuration has changed.

@HDembinski
Copy link
Member

Ah, but then the real issue is that m.errors should return 0 for fixed parameters? I agree that this would be nicer and more intuitive, but like you said it is not possible since m.errors has this double use as actual errors and initial step sizes. I think I cannot resolve this awkwardness, that originates from how C++ Minuit2 works.

For covariance matrix, where no such restrictions apply, the elements that correspond to fixed parameters are set to zero. So perhaps you could just change your workflow to use Minuit.covariance matrix instead of Minuit.errors. This way, you also get the correlations. To get the errors, do np.sqrt(np.diag(m.covariance)).

@HDembinski
Copy link
Member

What I would like to suggest considering is not applying the heuristic to parameters that are configured as fixed. The step size for those parameters does not matter for the fit, while in scenarios with multiple fits the heuristic could be applied before the next minimization whenever the .fixed configuration has changed.

That would be awkward in other ways. In OOP, we try to make interfaces so that the objects always contain a valid state. A valid state for a step size is a positive value, not a zero value. That zero or negative values were accepted in the past was an error of the API.

@HDembinski
Copy link
Member

I will try to implement #780 to make it a bit easier.

@alexander-held
Copy link
Member Author

Ah, but then the real issue is that m.errors should return 0 for fixed parameters?

That would be ideal I think, but as you describe seems not possible with the double use of m.errors. Just using the covariance matrix instead seems like a clean solution to work around this, thanks for the suggestion!

That would be awkward in other ways. In OOP, we try to make interfaces so that the objects always contain a valid state. A valid state for a step size is a positive value, not a zero value.

The zero step size is only invalid in my view with a configuration where the corresponding parameters are not also fixed. If such parameters were allowed to float again in subsequent fits, the step size could in principle be set again to nonzero for them.

Ultimately it does not make a big difference whether a user sets the step size to zero before minimization (which worked in the past) or sets the corresponding m.errors entries to zero after minimization (which has worked in the past and still does now). #780 looks useful in that regard, I tried using the m.errors[m.fixed] = 0 pattern described there at first and then instead ended up using np.where(m.fixed, 0.0, m.errors) which works but is perhaps less readable.

I originally opened this issue to understand whether this effect on fixed parameters was intentional, so for me everything here is now resolved. Thank you!

@HDembinski
Copy link
Member

The zero step size is only invalid in my view with a configuration where the corresponding parameters are not also fixed. If such parameters were allowed to float again in subsequent fits, the step size could in principle be set again to nonzero for them.

You are right. I am taking the liberty here to make the API a bit simpler by requiring that errors/step sizes must be always positive whether the parameter is fixed or not. I think that is easier to understand.

np.where(m.fixed, 0.0, m.errors)

I think that's a nice pattern actually. I don't like np.where very much, but this is a good use-case.

Thank you, too, for the discussion, I am closing this.

kratsg pushed a commit to scikit-hep/pyhf that referenced this issue Aug 9, 2022
* Manually set the uncertainties for fixed parameters to zero after minimization,
and remove setting the uncertainty/step_sizes during minimization, to avoid warnings
from iminuit. For iminuit v2.12.2+ "assigning zero to errors is invalid, but in the
past this was not caught." This approach harmonizes with cabinetry for fixed
parameters for iminuit v2.12.2+.
   - c.f. scikit-hep/iminuit#762
   - c.f. scikit-hep/cabinetry#346
   - c.f. https://iminuit.readthedocs.io/en/stable/changelog.html#july-15-2022
     > fix a bug in error heuristic when parameters have negative values and prevent
     > assigning negative values to errors
* Remove tests/test_optim.py test_step_sizes_fixed_parameters_minuit as the
uncertainties/step sizes values are no longer set during minimization and so are no
longer in pyhf's control.
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