fix: Correct handling of strategy 0 for Minuit optimizer

[wernerd-cern]

Description

The relevant line is supposed to set the strategy of minuit to one of the supported values 0, 1, or 2. Since the strategy 0 evaluates to False, this strategy couldn't be set when creating the optimizer if do_grad was False

pyhf/src/pyhf/optimize/opt_minuit.py

Lines 104 to 106 in f8f3c96

	strategy = options.pop(
	'strategy', self.strategy if self.strategy else not do_grad
	)

. The default value in the case of no specified strategy is None

pyhf/src/pyhf/optimize/opt_minuit.py

Line 39 in f8f3c96

self.strategy = kwargs.pop('strategy', None)

meaning that the statement is not None solves the issue with

self.strategy if self.strategy is not None else not do_grad

.

Amends PR #1183.

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

* Properly guard the `strategy` option for the Minuit optimizer in the case that
  `strategy` option of 0 is used.
  Without checking `self.strategy is not None` the `if self.strategy` for
  self.strategy==0 evaluates to truthiness of False, giving the wrong value of
  `not do_grad` for do_grad of False.
   - Amends PR https://github.com/scikit-hep/pyhf/pull/1183
* Use the corrected behavior to improve testing of default in test_minuit_strategy_global.
* Add Daniel Werner to contributors list.

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (f8f3c96) 98.30% compared to head (c0f4a97) 98.30%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2277   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          69       69           
  Lines        4534     4534           
  Branches      801      801           
=======================================
  Hits         4457     4457           
  Misses         45       45           
  Partials       32       32           

Flag	Coverage Δ
contrib	97.88% <ø> (ø)
doctest	61.07% <ø> (ø)
unittests-3.10	96.31% <ø> (ø)
unittests-3.11	96.31% <ø> (ø)
unittests-3.8	96.33% <ø> (ø)
unittests-3.9	96.36% <ø> (ø)

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[matthewfeickert]

Thanks for the PR @wernerd-cern. This should have been caught in the tests/test_optim.py tests, so we'll want to have a test added to this PR that covers this.

If you have time to add a test that will speed things up in getting this approved. If not, then the core devs can add one later this week and get this in.

[wernerd-cern]

In the case that no strategy is specified, it should default to not do_grad.
This is tested in the test test_minuit_strategy_do_grad.

In the test test_minuit_strategy_global a strategy is specified, when
the minimizer is set. In the case of strategy 0 and no strategy passed
to the fit function, however, the strategy defaults again to not do_grad.
This doesn't seem to be the desired behaviour as a strategy
was passed in the beginning. Therefore, I changed this assert to fail,
if the final strategy is different from the one assigned in the
beginning.

With this change one test case fails before applying the fix in this PR.
The PR fixes the test failure.
(The failing case is strategy 0 with the numpy backend as
default_do_grad is False for this backend.)

[matthewfeickert]

Thanks for your PR @wernerd-cern — your contributions are appreciated! This all LGTM. 👍

We'll go ahead and merge this in, and I'll work on getting a patch release out in hopefully the next day. Though given that this bug was found I think I should probably address Issue #1785 as well so that can go into the same patch release.

[matthewfeickert]

For future us, here's a simple example of what was going wrong:

# pr_2777_example.py

import itertools

strategies = [None, 0, 1]
grad_option = [False, True]

for strategy, do_grad in itertools.product(strategies, grad_option):
    print(f"\n# strategy: {strategy}")
    if strategy is not None:
        print(f"# do_grad: {do_grad}")
    else:
        print(f"# int(not do_grad): {int(not do_grad)}")

    print(f"v0.7.2:   {strategy if strategy else not do_grad}")
    print(f"PR #2277: {strategy if strategy is not None else not do_grad}")
    if strategy is None:
        print(
            f"PR #2277 cast to int: {strategy if strategy is not None else int(not do_grad)}"
        )

$ python pr_2777_example.py 

# strategy: None
# int(not do_grad): 1
v0.7.2:   True
PR #2277: True
PR #2277 cast to int: 1

# strategy: None
# int(not do_grad): 0
v0.7.2:   False
PR #2277: False
PR #2277 cast to int: 0

# strategy: 0
# do_grad: False
v0.7.2:   True
PR #2277: 0

# strategy: 0
# do_grad: True
v0.7.2:   False
PR #2277: 0

# strategy: 1
# do_grad: False
v0.7.2:   1
PR #2277: 1

# strategy: 1
# do_grad: True
v0.7.2:   1
PR #2277: 1

[matthewfeickert]

@wernerd-cern pyhf v0.7.3 is out now with this fix in it. Thanks very much for alerting us to this and for contributing a PR. 👍