fix: Guard Minuit optimizer against provided strategy of None #2278

matthewfeickert · 2023-08-15T23:18:45Z

Description

Guard Minuit optimizer strategy from None by first attempting to get a not-None result from the options kwargs and then falling back to assigning the strategy through int(not do_grad). In the process this simplifies the logic required and supersedes PR #2277.

Add tests to test_minuit_strategy_global for strategy=None and strategy=2 (2 is a valid iminuit.Minuit.strategy strategy but not used often, so done just for good measure).

Add a very brief explanation to docs of what using strategy=None means in terms of evaluation to 0 or 1 from int(pyhf.tensorlib.default_do_grad).

This extended example from @alexander-held in Issue #1785 now runs with this PR:

# issue-1785.py
import pyhf

model = pyhf.simplemodels.uncorrelated_background(
    signal=[12.0, 11.0], bkg=[50.0, 52.0], bkg_uncertainty=[3.0, 7.0]
)
data = [51, 48] + model.config.auxdata
pyhf.set_backend("numpy", "minuit")
pyhf.infer.mle.fit(data, model)

strategies = [None, 0, 1, 2]
for strategy in strategies:
    pyhf.infer.mle.fit(data, model, strategy=strategy)

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

* Guard Minuit optimizer strategy from None by first attempting to get a
  not-None result from the options kwargs and then falling back to
  assigning the strategy through the equivalent of `int(not do_grad)`.
  In the process this simplifies the logic required and supersedes PR
  https://github.com/scikit-hep/pyhf/pull/2277.
   - Cast the truthiness of `do_grad` as an integer for human
     readability in debugging.
* Update the comments on `do_grad` strategy with definitions from
  iMinuit v2.24.0 docs.
* Add tests to test_minuit_strategy_global for strategy=None and strategy=2
  (2 is a valid iminuit.Minuit.strategy strategy but not used often).
* Add brief explanation to docs of what using strategy=None means in
  terms of evaluation to 0 or 1 from `int(pyhf.tensorlib.default_do_grad)`.

* As the iminuit.Minuit.strategy is an integer, instead of having to have a human determine the integer representation of the inverse of the truthiness of do_grad during debugging or inspection do this in code.

src/pyhf/optimize/opt_minuit.py

matthewfeickert · 2023-08-15T23:22:32Z

tests/test_optim.py

@@ -173,27 +173,35 @@ def test_minuit_strategy_do_grad(mocker, backend):
    assert spy.spy_return.minuit.strategy == 1


-@pytest.mark.parametrize('strategy', [0, 1])
+@pytest.mark.parametrize('strategy', [0, 1, 2])


Adding 2 as we can and I forgot it existed.

matthewfeickert · 2023-08-15T23:23:59Z

Relevant RTD build: https://pyhf--2278.org.readthedocs.build/en/2278/_generated/pyhf.optimize.opt_minuit.minuit_optimizer.html

^ @alexander-held

matthewfeickert · 2023-08-15T23:24:25Z

src/pyhf/optimize/opt_minuit.py

+            strategy (:obj:`int`): See :attr:`iminuit.Minuit.strategy`.
+              Default is ``None``, which results in either
+              :attr:`iminuit.Minuit.strategy` ``0`` or ``1`` from the evaluation of
+              ``int(not pyhf.tensorlib.default_do_grad)``.


c.f. https://pyhf--2278.org.readthedocs.build/en/2278/_generated/pyhf.optimize.opt_minuit.minuit_optimizer.html for render.

codecov · 2023-08-15T23:30:08Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b81d903) 98.30% compared to head (6cd5366) 98.30%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2278   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          69       69           
  Lines        4534     4536    +2     
  Branches      801      802    +1     
=======================================
+ Hits         4457     4459    +2     
  Misses         45       45           
  Partials       32       32

Flag	Coverage Δ
contrib	`97.88% <100.00%> (+<0.01%)`	⬆️
doctest	`61.04% <0.00%> (-0.03%)`	⬇️
unittests-3.10	`96.31% <100.00%> (+<0.01%)`	⬆️
unittests-3.11	`96.31% <100.00%> (+<0.01%)`	⬆️
unittests-3.8	`96.34% <100.00%> (+<0.01%)`	⬆️
unittests-3.9	`96.36% <100.00%> (+<0.01%)`	⬆️

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% <100.00%> (ø)`

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

matthewfeickert · 2023-08-16T04:55:31Z

Once this is approved I'll make patch release pyhf v0.7.3 with this fix included in it.

matthewfeickert · 2023-08-16T04:56:36Z

src/pyhf/optimize/opt_minuit.py

+        # Guard against None from either self.strategy defaulting to None or
+        # passing strategy=None as options kwarg
+        if strategy is None:
+            strategy = int(not do_grad)


As the iminuit.Minuit.strategy is an integer, instead of having to have a human determine the integer representation of the inverse of the truthiness of do_grad during debugging or inspection do this in code.

If you're aiming for maximum readability, I like strategy = 0 if do_grad else 1 best.

alexander-held

This looks good to me, thanks! Just a tiny comment on what I believe is slightly more readable, but might be preference.

As a heads-up: there will also be a strategy=3 arriving eventually: root-project/root#13109 (more expensive and stable).

…ne (#2288) * Backport: - PR #2277 - PR #2278

* As the bug that was fixed in PR #2278 was subtle add an example to the pyhf v0.7.3 release notes that people can use to see if they were affected by it. * Amends PR #2290

matthewfeickert added 4 commits August 15, 2023 16:59

refactor: Use int of truthiness to improve readability

bc10ba4

* As the iminuit.Minuit.strategy is an integer, instead of having to have a human determine the integer representation of the inverse of the truthiness of do_grad during debugging or inspection do this in code.

test: Add strategy 2 to minuit optimizer tests

f634978

guard against Passing strategy=None to options

dcabf32

Provide explenation of what None gives

33d203a

matthewfeickert added docs Documentation related tests pytest fix A bug fix need-to-backport tmp label until can be backported to patch release branch labels Aug 15, 2023

matthewfeickert self-assigned this Aug 15, 2023

matthewfeickert commented Aug 15, 2023

View reviewed changes

matthewfeickert requested review from kratsg, lukasheinrich and alexander-held August 15, 2023 23:23

matthewfeickert commented Aug 15, 2023

View reviewed changes

kratsg approved these changes Aug 15, 2023

View reviewed changes

matthewfeickert added 4 commits August 15, 2023 22:28

Update defintions of iMinuit strategies

51a538a

Clarify comment

0cacda7

Simplify more

73307a5

Clarify comment even more given new simplification

6871bf1

matthewfeickert commented Aug 16, 2023

View reviewed changes

alexander-held reviewed Aug 16, 2023

View reviewed changes

Apply Alex's readability comment

6cd5366

matthewfeickert merged commit 4cadf91 into main Aug 16, 2023
19 checks passed

matthewfeickert deleted the fix/make-minuit-strategy-explicit branch August 16, 2023 08:36

matthewfeickert mentioned this pull request Aug 16, 2023

fix(backport): Guard Minuit optimizer against provided strategy of None #2288

Merged

4 tasks

matthewfeickert removed the need-to-backport tmp label until can be backported to patch release branch label Aug 16, 2023

matthewfeickert added a commit that referenced this pull request Aug 16, 2023

fix(backport): Guard Minuit optimizer against provided strategy of No…

dcc6e37

…ne (#2288) * Backport: - PR #2277 - PR #2278

matthewfeickert mentioned this pull request Aug 16, 2023

docs: Add v0.7.2 failing example to v0.7.3 release notes #2294

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Guard Minuit optimizer against provided strategy of None #2278

fix: Guard Minuit optimizer against provided strategy of None #2278

matthewfeickert commented Aug 15, 2023 •

edited

Loading

matthewfeickert Aug 15, 2023

matthewfeickert commented Aug 15, 2023

matthewfeickert Aug 15, 2023

codecov bot commented Aug 15, 2023 •

edited

Loading

matthewfeickert commented Aug 16, 2023

matthewfeickert Aug 16, 2023

alexander-held Aug 16, 2023

alexander-held left a comment

fix: Guard Minuit optimizer against provided strategy of None #2278

fix: Guard Minuit optimizer against provided strategy of None #2278

Conversation

matthewfeickert commented Aug 15, 2023 • edited Loading

Description

Checklist Before Requesting Reviewer

Before Merging

matthewfeickert Aug 15, 2023

Choose a reason for hiding this comment

matthewfeickert commented Aug 15, 2023

matthewfeickert Aug 15, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2023 • edited Loading

Codecov Report

matthewfeickert commented Aug 16, 2023

matthewfeickert Aug 16, 2023

Choose a reason for hiding this comment

alexander-held Aug 16, 2023

Choose a reason for hiding this comment

alexander-held left a comment

Choose a reason for hiding this comment

matthewfeickert commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading