feat: Use iminuit v2.0 API #1208

matthewfeickert · 2020-12-08T15:22:22Z

Description

Resolves #1166

Adopt the new iminuit v2.0 API.

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

* Require iminuit release compatible with v2.1
* Migrate to iminuit v2.0 API

src/pyhf/optimize/opt_minuit.py

matthewfeickert · 2020-12-11T09:54:45Z

For reasons I'm not 100% on at the moment test_optim.py is failing in a few places for torch one of them being

pyhf/tests/test_optim.py

Lines 229 to 239 in 924b86c

    
           def test_set_tolerance(backend): 
        
               m = pyhf.simplemodels.hepdata_like([50.0], [100.0], [10.0]) 
        
               data = pyhf.tensorlib.astensor([125.0] + m.config.auxdata) 
        
               assert pyhf.infer.mle.fit(data, m, tolerance=0.01) is not None 
        
               pyhf.set_backend(pyhf.tensorlib, pyhf.optimize.scipy_optimizer(tolerance=0.01)) 
        
               assert pyhf.infer.mle.fit(data, m) is not None 
        
               pyhf.set_backend(pyhf.tensorlib, pyhf.optimize.minuit_optimizer(tolerance=0.01)) 
        
               assert pyhf.infer.mle.fit(data, m) is not None

_______________________________________________________________________________________ test_set_tolerance[pytorch] ________________________________________________________________________________________

backend = (<pyhf.tensor.pytorch_backend.pytorch_backend object at 0x7fd480eb8280>, None)

    def test_set_tolerance(backend):
        m = pyhf.simplemodels.hepdata_like([50.0], [100.0], [10.0])
        data = pyhf.tensorlib.astensor([125.0] + m.config.auxdata)
    
        assert pyhf.infer.mle.fit(data, m, tolerance=0.01) is not None
    
        pyhf.set_backend(pyhf.tensorlib, pyhf.optimize.scipy_optimizer(tolerance=0.01))
        assert pyhf.infer.mle.fit(data, m) is not None
    
        pyhf.set_backend(pyhf.tensorlib, pyhf.optimize.minuit_optimizer(tolerance=0.01))
>       assert pyhf.infer.mle.fit(data, m) is not None

tests/test_optim.py:239: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/pyhf/infer/mle.py:123: in fit
    twice_nll, data, pdf, init_pars, par_bounds, fixed_vals, **kwargs
src/pyhf/optimize/mixins.py:136: in minimize
    result = self._internal_minimize(**minimizer_kwargs, options=kwargs)
src/pyhf/optimize/mixins.py:45: in _internal_minimize
    options=options,
src/pyhf/optimize/opt_minuit.py:132: in _minimize
    minimizer.migrad(ncall=maxiter)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = (Param(number=0, name='x0', value=1.0, error=0.01, merror=None, is_const=False, is_fixed=False, has_limits=True, has_l...lse, is_fixed=False, has_limits=True, has_lower_limit=True, has_upper_limit=True, lower_limit=1e-10, upper_limit=10.0))
ncall = 100000, iterate = 5

    def migrad(self, ncall=None, iterate=5):
        """Run MnMigrad from the Minuit2 library.
    
        MIGRAD is a robust minimisation algorithm which earned its reputation
        in 40+ years of almost exclusive usage in high-energy physics. How
        MIGRAD works is described in the `MINUIT paper`_.
    
        **Arguments:**
    
            * **ncall**: integer or None, optional; (approximate)
              maximum number of calls before minimization will be aborted. Default: None
              (indicates to use an internal heuristic). Note: The limit may be slightly
              violated, because the condition is checked only after a full iteration of
              the algorithm, which usually performs several function calls.
    
            * **iterate**: automatically call Migrad up to N times if convergence
              was not reached. Default: 5. This simple heuristic makes Migrad converge
              more often even if the numerical precision of the cost function is low.
              Setting this to 1 disables the feature.
    
        **Return:**
    
            self
        """
        if ncall is None:
            ncall = 0  # tells C++ Minuit to use its internal heuristic
    
        if iterate < 1:
            raise ValueError("iterate must be at least 1")
    
        migrad = MnMigrad(self._fcn, self._last_state, self.strategy)
    
        # Automatically call Migrad up to `iterate` times if minimum is not valid.
        # This simple heuristic makes Migrad converge more often.
        for _ in range(iterate):
            # workaround: precision must be set again after each call to MnMigrad
            if self._precision is not None:
                migrad.precision = self._precision
>           fm = migrad(ncall, self._tolerance)
E           RuntimeError: Unable to cast Python instance to C++ type (compile in debug mode for details)

../../../.venvs/pyhf-CPU/lib/python3.7/site-packages/iminuit/_minuit.py:547: RuntimeError

also from

_______________ test_optimizer_stitching[minuit-pytorch_backend] _______________

backend = <class 'pyhf.tensor.pytorch_backend.pytorch_backend'>
optimizer = 'minuit'

    @pytest.mark.parametrize(
        'backend',
        [
            pyhf.tensor.numpy_backend,
            pyhf.tensor.jax_backend,
            pyhf.tensor.tensorflow_backend,
            pyhf.tensor.pytorch_backend,
        ],
    )
    @pytest.mark.parametrize('optimizer', ['scipy', 'minuit'])
    def test_optimizer_stitching(backend, optimizer):
        pyhf.set_backend(backend(precision='64b'), optimizer)
    
        pdf = pyhf.simplemodels.hepdata_like([50.0], [100.0], [10])
        data = [125.0] + pdf.config.auxdata
    
>       result_nostitch = pyhf.infer.mle.fixed_poi_fit(2.0, data, pdf, do_stitch=False)

tests/test_validation.py:848: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/pyhf/infer/mle.py:190: in fixed_poi_fit
    return fit(data, pdf, init_pars, par_bounds, fixed_params, **kwargs)
src/pyhf/infer/mle.py:122: in fit
    return opt.minimize(
src/pyhf/optimize/mixins.py:136: in minimize
    result = self._internal_minimize(**minimizer_kwargs, options=kwargs)
src/pyhf/optimize/mixins.py:38: in _internal_minimize
    result = self._minimize(
src/pyhf/optimize/opt_minuit.py:129: in _minimize
    minimizer.migrad(ncall=maxiter)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = (Param(number=0, name='x0', value=2.0, error=0.0, merror=None, is_const=False, is_fixed=True, has_limits=True, has_low...lse, is_fixed=False, has_limits=True, has_lower_limit=True, has_upper_limit=True, lower_limit=1e-10, upper_limit=10.0))
ncall = 100000, iterate = 5

    def migrad(self, ncall=None, iterate=5):
        """Run MnMigrad from the Minuit2 library.
    
        MIGRAD is a robust minimisation algorithm which earned its reputation
        in 40+ years of almost exclusive usage in high-energy physics. How
        MIGRAD works is described in the `MINUIT paper`_.
    
        **Arguments:**
    
            * **ncall**: integer or None, optional; (approximate)
              maximum number of calls before minimization will be aborted. Default: None
              (indicates to use an internal heuristic). Note: The limit may be slightly
              violated, because the condition is checked only after a full iteration of
              the algorithm, which usually performs several function calls.
    
            * **iterate**: automatically call Migrad up to N times if convergence
              was not reached. Default: 5. This simple heuristic makes Migrad converge
              more often even if the numerical precision of the cost function is low.
              Setting this to 1 disables the feature.
    
        **Return:**
    
            self
        """
        if ncall is None:
            ncall = 0  # tells C++ Minuit to use its internal heuristic
    
        if iterate < 1:
            raise ValueError("iterate must be at least 1")
    
        migrad = MnMigrad(self._fcn, self._last_state, self.strategy)
    
        # Automatically call Migrad up to `iterate` times if minimum is not valid.
        # This simple heuristic makes Migrad converge more often.
        for _ in range(iterate):
            # workaround: precision must be set again after each call to MnMigrad
            if self._precision is not None:
                migrad.precision = self._precision
>           fm = migrad(ncall, self._tolerance)
E           RuntimeError: Unable to cast Python instance to C++ type (compile in debug mode for details)

/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/site-packages/iminuit/_minuit.py:547: RuntimeError

@HDembinski Any thoughts on why this seems to be happening for torch only? (I'll add the caveat it is late at night for me now so I may be missing something trivial).

matthewfeickert · 2020-12-11T09:54:56Z

@kratsg We're also going to need to revise these tests:

pyhf/tests/test_optim.py

Lines 496 to 517 in 924b86c

    
           def test_init_pars_sync_fixed_values_minuit(mocker): 
        
               opt = pyhf.optimize.minuit_optimizer() 
        
               # patch all we need 
        
               from pyhf.optimize import opt_minuit 
        
               minimizer = mocker.patch.object(opt_minuit, 'iminuit') 
        
               opt._get_minimizer(None, [9, 9, 9], [(0, 10)] * 3, fixed_vals=[(0, 1)]) 
        
               assert minimizer.Minuit.from_array_func.call_args[1]['start'] == [1, 9, 9] 
        
               assert minimizer.Minuit.from_array_func.call_args[1]['fix'] == [True, False, False] 
        
           def test_step_sizes_fixed_parameters_minuit(mocker): 
        
               opt = pyhf.optimize.minuit_optimizer() 
        
               # patch all we need 
        
               from pyhf.optimize import opt_minuit 
        
               minimizer = mocker.patch.object(opt_minuit, 'iminuit') 
        
               opt._get_minimizer(None, [9, 9, 9], [(0, 10)] * 3, fixed_vals=[(0, 1)]) 
        
               assert minimizer.Minuit.from_array_func.call_args[1]['fix'] == [True, False, False] 
        
               assert minimizer.Minuit.from_array_func.call_args[1]['error'] == [0.0, 0.01, 0.01]

The good news is that we're down to just two test sets that are failing:

tests/test_optim.py
tests/test_validation.py

and I think all the (known) errors are described above here, so the set of failures is reduced to a small targeted list.

tests/test_optim.py

HDembinski · 2020-12-11T20:09:13Z

Does not look trivial, but I have very little time over the weekend to look into this. I can have a look next week.

HDembinski · 2020-12-11T20:12:21Z

You can try what happens when you call Minuit.fcn(parameter_values). If you get the error, then that would narrow it down. (Replace Minuit with your object instance).

matthewfeickert · 2020-12-11T20:12:47Z

I can have a look next week.

Thanks @HDembinski. This shouldn't be a high priority for you, but when you do get time for it please let us know if we can help. 👍

HDembinski · 2020-12-11T20:15:10Z

I want this to work in pyhf, I am eager to help, but I am very short on time right now. Thanks for adopting so quickly!

matthewfeickert · 2020-12-11T20:21:45Z

I want this to work in pyhf, I am eager to help, but I am very short on time right now.

We fully understand and appreciate this 🙂 — I think we're all rather impressed with how much development work you're getting done right now when everyone is so busy at the end of the year.

HDembinski · 2020-12-14T19:53:19Z

I think we're all rather impressed with how much development work you're getting done right now when everyone is so busy at the end of the year.

It is how I procrastinate from other work.

HDembinski · 2020-12-14T20:13:18Z

I checked out this branch locally. pip install -e . seemed to work. I cannot run tests yet, it complains that pytorch is missing, which I am currently installing (800MB download, seriously?!)

HDembinski · 2020-12-14T20:19:39Z

Ok, got pytorch, now when trying to run tests/test_optim.py, it complains that tensorflow is not installed. If these are optional backends, shouldn't the tests just skip when they are not installed?

HDembinski · 2020-12-14T20:44:25Z

It is only happening when analytical gradients are used.
I compiled iminuit locally in DEBUG mode (just run make in the iminuit repo) and that gives more information:
E RuntimeError: Unable to cast Python instance of type <class 'torch.Tensor'> to C++ type 'std::vector<double, std::allocator<double> >'

I let pybind11 do the conversion. I am not sure how exactly the cast works, but I guess it expects the type to support the PySequence protocol, which torch.Tensor perhaps does not. I can try to work around that.

matthewfeickert · 2020-12-14T20:54:51Z

I checked out this branch locally. pip install -e . seemed to work

Okay great. I'll rebase this against master so that this branch also knows about the pytest fix from PR #1220.

It is only happening when analytical gradients are used.

I compiled iminuit locally in DEBUG mode (just run make in the iminuit repo) and that gives more information:
E RuntimeError: Unable to cast Python instance of type <class 'torch.Tensor'> to C++ type 'std::vector<double, std::allocator<double> >'

I let pybind11 do the conversion. I am not sure how exactly the cast works, but I guess it expects the type to support the PySequence protocol, which torch.Tensor perhaps does not. I can try to work around that.

Okay let us know if there is any information that we can provide.

If these are optional backends, shouldn't the tests just skip when they are not installed?

Perhaps, but as we don't ship the tests with the code we assume that the dev team is going to be running the full test suite locally and we want to know that all the backends agree. So I guess I would say that the backends are optional for runtime but not optional for the dev team.

HDembinski · 2020-12-14T21:29:37Z

Indeed, torch.Tensor does not register as a Sequence. isinstance(some_tensor, collections.abc.Sequence) is False. The sequence protocol is required by pybind11. Now, there are three ways to fix this.

in iminuit: I make a custom caster that also accepts torch.Tensor or rather everything that supports the buffer protocol.
in pybind11: you could argue that their caster does not try hard enough (there are collections which are not sequences and which can still be converted to std::vector)
in pyhf: you could convert the tensor to a numpy array in your gradient function wrapper.

matthewfeickert · 2020-12-14T21:43:29Z

Indeed, torch.Tensor does not register as a Sequence. isinstance(some_tensor, collections.abc.Sequence) is False. The sequence protocol is required by pybind11. Now, there are three ways to fix this.

in iminuit: I make a custom caster that also accepts torch.Tensor or rather everything that supports the buffer protocol.

in pybind11: you could argue that their caster does not try hard enough (there are collections which are not sequences and which can still be converted to std::vector)

in pyhf: you could convert the tensor to a numpy array in your gradient function wrapper.

Thanks for this diagnosis @HDembinski. I think we should avoid you doing any extra work if possible and I believe that we can just case to NumPy here without any issue. I'll let you know if we end up running into additional issues, but I'll work on implementing the

in pyhf: you could convert the tensor to a numpy array in your gradient function wrapper.

option.

HDembinski · 2020-12-14T22:22:48Z

I am working on the implementation for iminuit. Having a custom caster allows me to improve performance a bit.

HDembinski · 2020-12-14T22:42:42Z

@matthewfeickert I checked that my patch scikit-hep/iminuit#574 fixes the issue.

HDembinski · 2020-12-14T23:08:23Z

It is now fixed in the iminuit develop branch.

kratsg

All looks good generally. Note that we mock out minuit since we don't need to use it to check for behavior from it. We're checking that the functions we've implemented handle the fixed values/init values correspondingly.

kratsg

Just realized we do want to mock out iminuit since we need to check functionality of our code, rather than of minuit's code in these tests.

* from CHANGELOG: Minuit.np_covariance is now obsolete and was removed; see Minuit.covariance

* Minuit.np_values was removed; use Minuit.values instead or np.array(m.values)

* Minuit.np_errors was removed; use Minuit.errors instead or np.array(m.errors)

tests/test_optim.py

matthewfeickert added feat/enhancement New feature or request Scikit-HEP Scikit-HEP operations and projects labels Dec 8, 2020

matthewfeickert self-assigned this Dec 8, 2020

matthewfeickert commented Dec 8, 2020

View reviewed changes

src/pyhf/optimize/opt_minuit.py Outdated Show resolved Hide resolved

matthewfeickert force-pushed the feat/migrate-to-iminuit-v2.0-api branch from 4944bdf to 2d95cac Compare December 11, 2020 08:51

matthewfeickert commented Dec 11, 2020

View reviewed changes

tests/test_optim.py Outdated Show resolved Hide resolved

matthewfeickert force-pushed the feat/migrate-to-iminuit-v2.0-api branch from 1f7aacf to 605d10d Compare December 14, 2020 20:55

This was referenced Dec 14, 2020

Participate in iminuit v2.0 beta? #1166

Closed

Add .to_numpy() API to tensorlib #1225

Closed

HDembinski mentioned this pull request Dec 14, 2020

use custom type_caster for std::vector scikit-hep/iminuit#574

Merged

matthewfeickert mentioned this pull request Dec 15, 2020

ci: Uninstall dependencies before installed HEAD versions #1227

Merged

4 tasks

matthewfeickert force-pushed the feat/migrate-to-iminuit-v2.0-api branch from f087df6 to 5bc5e5b Compare December 15, 2020 04:32

kratsg approved these changes Jan 4, 2021

View reviewed changes

kratsg requested changes Jan 4, 2021

View reviewed changes

matthewfeickert and others added 17 commits January 4, 2021 12:24

Change ncalls -> nfcn

b514d14

Change ngrads -> ngrad

ad8e49d

Use ncalls_total and ngrads_total API

1619308

Go back to nfcn and ngrad

e8ce51e

apply iminuit PRs 518 and 519

94b9da4

Simply try to pass all to Minuit init

e7cdbc8

Try to pass init pars in correctly to *args

e01cebc

Use iminuit v2.0

a1efb0e

Set values through OO API now

a85ab7e

np_covariance -> covariance

3ea9993

* from CHANGELOG: Minuit.np_covariance is now obsolete and was removed; see Minuit.covariance

np_values -> values

bd802d3

* Minuit.np_values was removed; use Minuit.values instead or np.array(m.values)

np_errors -> errors

f48ce95

* Minuit.np_errors was removed; use Minuit.errors instead or np.array(m.errors)

First try at revising tests

d5dc1f3

Remove old code

6baee20

Add notes on failing tests

7f6d3d4

Get patch release v2.1 to fix

8fdecb5

fix up tests to mock

decd4ad

kratsg force-pushed the feat/migrate-to-iminuit-v2.0-api branch from 54fd990 to decd4ad Compare January 4, 2021 17:25

matthewfeickert commented Jan 5, 2021

View reviewed changes

tests/test_optim.py Outdated Show resolved Hide resolved

fix up 3.6

3020e05

kratsg approved these changes Jan 5, 2021

View reviewed changes

kratsg merged commit a72104e into master Jan 5, 2021

kratsg deleted the feat/migrate-to-iminuit-v2.0-api branch January 5, 2021 12:54

alexander-held mentioned this pull request Jan 5, 2021

Changes for pyhf 0.6.0 scikit-hep/cabinetry#150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use iminuit v2.0 API #1208

feat: Use iminuit v2.0 API #1208

matthewfeickert commented Dec 8, 2020 •

edited

Loading

matthewfeickert commented Dec 11, 2020

matthewfeickert commented Dec 11, 2020 •

edited

Loading

HDembinski commented Dec 11, 2020

HDembinski commented Dec 11, 2020 •

edited

Loading

matthewfeickert commented Dec 11, 2020

HDembinski commented Dec 11, 2020

matthewfeickert commented Dec 11, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020 •

edited

Loading

matthewfeickert commented Dec 14, 2020 •

edited

Loading

HDembinski commented Dec 14, 2020 •

edited

Loading

matthewfeickert commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

kratsg left a comment

kratsg left a comment

feat: Use iminuit v2.0 API #1208

feat: Use iminuit v2.0 API #1208

Conversation

matthewfeickert commented Dec 8, 2020 • edited Loading

Description

Checklist Before Requesting Reviewer

Before Merging

matthewfeickert commented Dec 11, 2020

matthewfeickert commented Dec 11, 2020 • edited Loading

HDembinski commented Dec 11, 2020

HDembinski commented Dec 11, 2020 • edited Loading

matthewfeickert commented Dec 11, 2020

HDembinski commented Dec 11, 2020

matthewfeickert commented Dec 11, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020 • edited Loading

matthewfeickert commented Dec 14, 2020 • edited Loading

HDembinski commented Dec 14, 2020 • edited Loading

matthewfeickert commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

HDembinski commented Dec 14, 2020

kratsg left a comment

Choose a reason for hiding this comment

kratsg left a comment

Choose a reason for hiding this comment

matthewfeickert commented Dec 8, 2020 •

edited

Loading

matthewfeickert commented Dec 11, 2020 •

edited

Loading

HDembinski commented Dec 11, 2020 •

edited

Loading

HDembinski commented Dec 14, 2020 •

edited

Loading

matthewfeickert commented Dec 14, 2020 •

edited

Loading

HDembinski commented Dec 14, 2020 •

edited

Loading