fix: Manually set uncertainties for fixed parameters to 0

[matthewfeickert]

Description

Resolves #1918

• 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. Step size of fixed parameters changes after v2.12.1 due to heuristic being applied iminuit#762
  • c.f. feat: explicitly set post-fit parameter errors to 0 for fixed parameters 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.

Checklist Before Requesting Reviewer

• Tests are passing
• "WIP" removed from the title of the pull request
• Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

• Summarize commit messages into a comprehensive review of the PR

* 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. https://github.com/scikit-hep/iminuit/issues/762
   - c.f. https://github.com/scikit-hep/cabinetry/pull/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.

[codecov]

Codecov Report

Merging #1919 (db7a7d7) into master (8e242c9) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1919      +/-   ##
==========================================
- Coverage   98.17%   98.17%   -0.01%     
==========================================
  Files          68       68              
  Lines        4331     4330       -1     
  Branches      730      729       -1     
==========================================
- Hits         4252     4251       -1     
  Misses         46       46              
  Partials       33       33              

Flag	Coverage Δ
contrib	26.37% <75.00%> (+0.02%)	⬆️
doctest	60.66% <75.00%> (+0.03%)	⬆️
unittests-3.10	96.05% <100.00%> (-0.01%)	⬇️
unittests-3.7	96.03% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/pyhf/optimize/opt_minuit.py	100.00% <ø> (ø)
src/pyhf/optimize/mixins.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[matthewfeickert]

@kratsg @lukasheinrich I'm going to leave this in draft until I have enough time to look at this and scikit-hep/iminuit#762 and determine if the change in uncertainties is desired and create some minimal examples that demonstrate the differences.

[alexander-held]

Based on scikit-hep/iminuit#762 (comment),

pyhf/src/pyhf/optimize/opt_minuit.py

Line 53 in 2cd8c0b

step_sizes = [(b[1] - b[0]) / float(self.steps) for b in init_bounds]

could also in principle be removed I think (does not need to happen here, but came up in the same context, and presumably the effect is negligible).

I am not yet sure how to handle this in cabinetry but I am leaning towards going through the fit results and manually setting errors for fixed parameters to 0.0 again. I will put my motivation for that into scikit-hep/iminuit#762. It would be best to harmonize the behavior of pyhf and cabinetry though so I'm also interested to hear your thoughts on this so we can ideally agree on the approach.

[matthewfeickert]

Note to self in case needed in the future: Previous commit message summary was

• Update lower bound on iminuit to v2.12.2 as it is the 'oldest' SemVer release to ensure uncertainties are positive.
  • 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

• Update the tests/test_optim.py test_optim_uncerts uncertainties values to be nonzero as "assigning zero to errors is invalid, but in the past this was not caught."
  • c.f. Step size of fixed parameters changes after v2.12.1 due to heuristic being applied iminuit#762
• Update tests/constraints.txt to use iminuit==2.12.2.
• Add a ignore to filterwarnings to avoid iminuit UserWarning:

UserWarning: Assigned errors must be positive. Non-positive values are replaced by a heuristic.

This was introduced in iminuit v2.12.2 as the result of a step size guess heuristic being applied to fixed parameters. This results in the error of fixed parameters changing during MIGRAD.

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

[alexander-held]

I think the current implementation allows for leaving the iminuit version requirement unchanged. The change in 2.12.2 affects parameters with an assigned step size of zero. Since pyhf no longer sets a step size of zero to any parameters (and instead corrects the post-fit parameter uncertainties for fixed parameters now), iminuit versions before and after 2.12.2 should behave the same in combination with these changes to pyhf.

The one thing that might break, but for which I do not see a good solution, is user code that directly uses the Minuit instance, reads out .errors on that and gets a different behavior starting with 2.12.2. I don't think pyhf can (or should) do anything about that though.

[matthewfeickert]

I think the current implementation allows for leaving the iminuit version requirement unchanged. The change in 2.12.2 affects parameters with an assigned step size of zero. Since pyhf no longer sets a step size of zero to any parameters (and instead corrects the post-fit parameter uncertainties for fixed parameters now), iminuit versions before and after 2.12.2 should behave the same in combination with these changes to pyhf.

The one thing that might break, but for which I do not see a good solution, is user code that directly uses the Minuit instance, reads out .errors on that and gets a different behavior starting with 2.12.2. I don't think pyhf can (or should) do anything about that though.

Yeah the only reason I included the version bump was to ensure that there could only be one behavior if the user tried to access Minuit.errors, but I think I agree that I'm trying to be too protective of the user in this instance and if they are running into this that we should just direct them to Issue scikit-hep/iminuit#762.

[alexander-held]

Yeah the only reason I included the version bump was to ensure that there could only be one behavior if the user tried to access Minuit.errors

The behavior should still be the same with old and new versions, i.e. non-zero .errors for fixed parameters (since you're no longer setting the zero value before the fit). A change appears though for someone installing e.g. pyhf 0.6.3 with old and new versions of iminuit (I think that change cannot be prevented from the side of pyhf though).

[matthewfeickert]

A change appears though for someone installing e.g. pyhf 0.6.3 with old and new versions of iminuit (I think that change cannot be prevented from the side of pyhf though).

Exactly. Though this [not being able to force users to install newer versions software] is what you, Henry, Hans, and Thomas have all reminded me about many times and that I have to let users make their own decisions.