Participate in iminuit v2.0 beta? #1166

HDembinski · 2020-11-06T20:52:13Z

Dear pyhf team,

I am about to finish a major rewrite of iminuit, version 2.0, that replaces Cython as the tool to wrap C++ Minuit2 with pybind11, which is going to solve several issues that the legacy code had. All the good things that this will bring are listed on top of this PR:
scikit-hep/iminuit#502

Switching to the new version of iminuit should be completely transparent to you, since the new version passes the comprehensive suite of unit tests of iminuit-v1.x. However, I would like to use this opportunity to finally remove interface that has been successively marked as deprecated in versions 1.3 to 1.5.

Therefore my two question to you:

Did you take note of the deprecation warnings in iminuit and did you keep up with the interface changes so far?
Are you interested in trying out a Beta release of v2.0 to work out any possible bugs in the new version before the release?

Best regards,
Hans, iminuit maintainer

matthewfeickert · 2020-11-06T21:31:29Z

Hi @HDembinski. Thanks for thinking of us and giving us this heads up.

I am about to finish a major rewrite of iminuit, version 2.0, that replaces Cython as the tool to wrap C++ Minuit2 with pybind11, which is going to solve several issues that the legacy code had.

This is very cool to hear. Congratulations!

Did you take note of the deprecation warnings in iminuit and did you keep up with the interface changes so far?

I know that I don't do a very good job of checking the warnings, but we can do some checks to make sure we're not hitting any.

Are you interested in trying out a Beta release of v2.0 to work out any possible bugs in the new version before the release?

Very much so. We have a nightly workflow that runs the test suite using an install from the HEAD of the main branch of iminuit on GitHub (develop I think for you), so we should hit anything before release and might already be testing your v2.0 beta.

pyhf/.github/workflows/dependencies-head.yml

Lines 35 to 55 in a3b34a5

    
             iminuit: 
        
               runs-on: ${{ matrix.os }} 
        
               strategy: 
        
                 matrix: 
        
                   os: [ubuntu-latest] 
        
                   python-version: [3.8] 
        
               steps: 
        
               - uses: actions/checkout@v2 
        
               - name: Set up Python ${{ matrix.python-version }} 
        
                 uses: actions/setup-python@v2 
        
                 with: 
        
                   python-version: ${{ matrix.python-version }} 
        
               - name: Install dependencies 
        
                 run: | 
        
                   python -m pip install --upgrade pip setuptools wheel 
        
                   python -m pip install --ignore-installed --upgrade -q --no-cache-dir -e .[test] 
        
                   python -m pip install --upgrade --no-cache-dir cython 
        
                   python -m pip install --upgrade --no-cache-dir git+git://github.com/scikit-hep/iminuit.git 
        
                   python -m pip list

If you already have a branch beyond the main develop that this beta version is on, then we can just add another workflow that catches that.

kratsg · 2020-11-06T21:47:41Z

Did you take note of the deprecation warnings in iminuit and did you keep up with the interface changes so far?

we took care of them all generally with the major rewrite of our underlying optimizer api.

HDembinski · 2020-11-08T18:47:51Z

Thank you! Since you are testing develop, you should already get the latest, but you can also install the beta from test.pypi:
pip install -i https://test.pypi.org/simple/ iminuit

matthewfeickert · 2020-11-09T06:53:28Z

Thank you! Since you are testing develop, you should already get the latest

Yup! We're already starting to see tests fail there (AttributeError: 'Minuit' object has no attribute 'ncalls'), so it seems things are working nicely in terms of giving us early warning. We'll go ahead and setup a branch that we can start to do tests on that moves to using the new API.

HDembinski · 2020-11-09T19:43:34Z

@matthewfeickert Thank you for the feedback, but I am thoroughly confused. This looks like you are calling minimize, which should of course work (I am testing it) and it is adapted to the new interface.

HDembinski · 2020-11-09T19:46:52Z

Ah, it looks like you either reimplemented minimize from iminuit or you simply have a function with the same name that also returns a scipy OptimizeResult

lukasheinrich · 2020-11-09T19:47:30Z

we conostruct Minuit objects here:

https://github.com/scikit-hep/pyhf/blob/master/src/pyhf/optimize/opt_minuit.py#L67

HDembinski · 2020-11-09T19:47:42Z

ncalls has been renamed to nfcn and ngrads to ngrad for consistency.

matthewfeickert · 2020-11-09T20:08:29Z

ncalls has been renamed to nfcn and ngrads to ngrad for consistency.

Hm. I'll have to look at this more later, as running the "HEAD of dependencies" workflow with those changes to this block

pyhf/src/pyhf/optimize/opt_minuit.py

Lines 122 to 132 in 28fdfe9

    
           return scipy.optimize.OptimizeResult( 
        
               x=minimizer.np_values(), 
        
               unc=unc, 
        
               success=minimizer.valid, 
        
               fun=minimizer.fval, 
        
               hess_inv=hess_inv, 
        
               message=message, 
        
               nfev=minimizer.ncalls, 
        
               njev=minimizer.ngrads, 
        
               minuit=minimizer, 
        
           )

on branch feat/migrate-to-iminuit-v2.0-api is now giving a AttributeError: 'Minuit' object has no attribute 'nfcn'.

matthewfeickert · 2020-11-09T20:22:28Z

Oh. Looking at https://github.com/scikit-hep/iminuit/blob/2503a042a292657d573912bb64a3407df76ada89/src/iminuit/_minimize.py#L112-L121 it seems that we want ncalls -> ncalls_total and ngrads -> ngrads_total.

matthewfeickert · 2020-11-09T20:43:34Z

Hm. Still getting AttributeErrors so I'll need to do a better job of comparing the difference between https://github.com/scikit-hep/iminuit/blob/v1.4.9/src/iminuit/_minimize.py and https://github.com/scikit-hep/iminuit/blob/2503a042a292657d573912bb64a3407df76ada89/src/iminuit/_minimize.py

kratsg · 2020-11-09T20:53:00Z

Ah, it looks like you either reimplemented minimize from iminuit or you simply have a function with the same name that also returns a scipy OptimizeResult

I mostly ripped out some of the python-facing code from iminuit to write up the optimizer wrapper here - so you're right. I liked the idea that you provided a OptimizeResult object, and tried to replicate that for the scipy implementation we have. As mentioned in the comment, I grabbed it from https://github.com/scikit-hep/iminuit/blob/22f6ed7146c1d1f3274309656d8c04461dde5ba3/src/iminuit/_minimize.py#L106-L125 which is what the wrapped was based off of.

Mainly, we needed a bit more control on instantiation of the object vs actually running the fit (so it was split up this way).

HDembinski · 2020-11-09T21:33:46Z

I think this shows that the counters are still messy and confusing. I am going to remove ncalls_total and ngrads_total. Please use Minuit.nfcn and Minuit.ngrad to get the same. See scikit-hep/iminuit#514

HDembinski · 2020-11-09T21:42:07Z

The develop branch had a bug, which I didn't detect before. I merged scikit-hep/iminuit#514, which fixes the bug, but you still need to change the counter names as described in my previous message.

matthewfeickert · 2020-11-09T22:27:57Z

I merged scikit-hep/iminuit#514, which fixes the bug, but you still need to change the counter names as described in my previous message.

Okay, with that we're now passing that section but failing

pyhf/tests/test_optim.py

Line 47 in 28fdfe9

def test_minimize(tensorlib, precision, optimizer, do_grad, do_stitch):

with

>           result = pyhf.infer.mle.fit(data, m, do_grad=do_grad, do_stitch=do_stitch)

tests/test_optim.py:105: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
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:101: in _minimize
    minimizer.migrad(ncall=maxiter)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <iminuit._minuit.Minuit object at 0x7f57644a9f40>, ncall = 100000
precision = None, iterate = 5

    def migrad(self, ncall=None, precision=None, iterate=5):
        """Run MIGRAD.
    
        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 call before MIGRAD will stop trying. Default: None
              (indicates to use MIGRAD's internal heuristic). Note: MIGRAD may slightly
              violate this limit, because it checks the condition only after a full
              iteration of the algorithm, which usually performs several function calls.
    
            * **precision**: override Minuit precision estimate for the cost function.
              Default: None (= use epsilon of a C++ double). If the cost function has a
              lower precision (e.g. of a C++ float), setting this to a lower value will
              accelerate convergence and reduce the rate of unsuccessful convergence.
    
            * **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:**
    
            :ref:`function-minimum-sruct`, list of :ref:`minuit-param-struct`
        """
        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)
        migrad.set_print_level(self.print_level)
        if precision is not None:
            migrad.precision = precision
    
        # Automatically call Migrad up to `iterate` times if minimum is not valid.
        # This simple heuristic makes Migrad converge more often.
        for _ in range(iterate):
>           fm = migrad(ncall, self.tol)
E           RuntimeError: Unable to cast Python instance to C++ type (compile in debug mode for details)

HDembinski · 2020-11-12T19:07:11Z

I am not sure what is going on. ncall seems to be a number which is ok, then it can only be self.tol that is set to something that is not convertible to double.

matthewfeickert · 2020-11-19T04:40:21Z

@HDembinski I'm now also seeing in runs

    def test_hypotest_backends(backend, kwargs):
        """
        Check that hypotest runs fully across all backends for all calculator types.
        """
        pdf = pyhf.simplemodels.hepdata_like(
            signal_data=[12.0, 11.0], bkg_data=[50.0, 52.0], bkg_uncerts=[3.0, 7.0]
        )
        data = [51, 48] + pdf.config.auxdata
>       assert pyhf.infer.hypotest(1.0, data, pdf, **kwargs) is not None

tests/test_infer.py:165: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/pyhf/infer/__init__.py:146: in hypotest
    teststat = calc.teststatistic(poi_test)
src/pyhf/infer/calculators.py:258: in teststatistic
    qmu_v = teststat_func(
src/pyhf/infer/test_statistics.py:177: in qmu_tilde
    return _qmu_like(mu, data, pdf, init_pars, par_bounds, fixed_params)
src/pyhf/infer/test_statistics.py:19: in _qmu_like
    tmu_like_stat, (_, muhatbhat) = _tmu_like(
src/pyhf/infer/test_statistics.py:38: in _tmu_like
    mubhathat, fixed_poi_fit_lhood_val = fixed_poi_fit(
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:35: in _internal_minimize
    minimizer = self._get_minimizer(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <pyhf.optimize.minuit_optimizer object at 0x7f3fa6e957c0>
objective_and_grad = <function wrap_objective.<locals>.func at 0x7f3f84139b80>
init_pars = [1.0, 1.0, 1.0]
init_bounds = [(0, 10), (1e-10, 10.0), (1e-10, 10.0)], fixed_vals = [(0, 1.0)]
do_grad = False

    def _get_minimizer(
        self, objective_and_grad, init_pars, init_bounds, fixed_vals=None, do_grad=False
    ):
    
        step_sizes = [(b[1] - b[0]) / float(self.steps) for b in init_bounds]
        fixed_vals = fixed_vals or []
        # Minuit wants True/False for each parameter
        fixed_bools = [False] * len(init_pars)
        for index, val in fixed_vals:
            fixed_bools[index] = True
            init_pars[index] = val
            step_sizes[index] = 0.0
    
        # Minuit requires jac=callable
        if do_grad:
            wrapped_objective = lambda pars: objective_and_grad(pars)[0]
            jac = lambda pars: objective_and_grad(pars)[1]
        else:
            wrapped_objective = objective_and_grad
            jac = None
    
        kwargs = dict(
            fcn=wrapped_objective,
            grad=jac,
            start=init_pars,
            error=step_sizes,
            limit=init_bounds,
            fix=fixed_bools,
            print_level=self.verbose,
            errordef=self.errordef,
        )
>       return iminuit.Minuit.from_array_func(**kwargs)
E       AttributeError: type object 'Minuit' has no attribute 'from_array_func'

It looks like scikit-hep/iminuit#526 removed from_array_func, so is there a new API that will allow for similar behavior to iminuit.Minuit.from_array_func(**kwargs)?

alexander-held · 2020-11-19T09:59:46Z

I think the replacement for iminuit.Minuit.from_array_func(...) will just be iminuit.Minuit(...) scikit-hep/iminuit#519. Note also scikit-hep/iminuit#518, which will cause some changes since less things can be set in the constructor now.

HDembinski · 2020-11-19T10:40:38Z

Yes, you can just use init now. Minuit(fcn_np, array_of_starting_values)

matthewfeickert · 2020-11-30T22:33:57Z

The 2.0 beta changelog is helpful here as well: https://iminuit.readthedocs.io/en/latest/changelog.html#id2

HDembinski · 2020-12-07T09:59:44Z

Right, at the time when @alexander-held asked the question, there was no changelog yet.

HDembinski · 2020-12-08T16:57:13Z

iminuit v2.0 is released, the breaking changes in the interface are documented in the changelog which can serve as an upgrade guide.

matthewfeickert · 2020-12-11T10:04:18Z

Cross-posting with the PR:

_______________ 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

matthewfeickert · 2020-12-14T21:47:15Z

From PR #1208

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

@HDembinski, while we said that we'll work on casting to NumPy for the time being, I'll add that this might be nice down the road to have in iminuit if possible if there would be other people out there (thinking of the ML community) that would be naturally working in torch and want to leverage iminuit for other analysis in the same program.

HDembinski · 2020-12-14T22:45:15Z

Agreed.

matthewfeickert added the research experimental stuff label Nov 6, 2020

matthewfeickert added the tests pytest label Nov 6, 2020

matthewfeickert self-assigned this Nov 19, 2020

matthewfeickert added the Scikit-HEP Scikit-HEP operations and projects label Dec 6, 2020

matthewfeickert mentioned this issue Dec 8, 2020

feat: Use iminuit v2.0 API #1208

Merged

4 tasks

kratsg closed this as completed in #1208 Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Participate in iminuit v2.0 beta? #1166

Participate in iminuit v2.0 beta? #1166

HDembinski commented Nov 6, 2020

matthewfeickert commented Nov 6, 2020 •

edited

Loading

kratsg commented Nov 6, 2020

HDembinski commented Nov 8, 2020

matthewfeickert commented Nov 9, 2020 •

edited

Loading

HDembinski commented Nov 9, 2020

HDembinski commented Nov 9, 2020

lukasheinrich commented Nov 9, 2020

HDembinski commented Nov 9, 2020

matthewfeickert commented Nov 9, 2020 •

edited

Loading

matthewfeickert commented Nov 9, 2020

matthewfeickert commented Nov 9, 2020

kratsg commented Nov 9, 2020

HDembinski commented Nov 9, 2020

HDembinski commented Nov 9, 2020 •

edited

Loading

matthewfeickert commented Nov 9, 2020

HDembinski commented Nov 12, 2020

matthewfeickert commented Nov 19, 2020

alexander-held commented Nov 19, 2020

HDembinski commented Nov 19, 2020

matthewfeickert commented Nov 30, 2020

HDembinski commented Dec 7, 2020

HDembinski commented Dec 8, 2020

matthewfeickert commented Dec 11, 2020

matthewfeickert commented Dec 14, 2020

HDembinski commented Dec 14, 2020

Participate in iminuit v2.0 beta? #1166

Participate in iminuit v2.0 beta? #1166

Comments

HDembinski commented Nov 6, 2020

matthewfeickert commented Nov 6, 2020 • edited Loading

kratsg commented Nov 6, 2020

HDembinski commented Nov 8, 2020

matthewfeickert commented Nov 9, 2020 • edited Loading

HDembinski commented Nov 9, 2020

HDembinski commented Nov 9, 2020

lukasheinrich commented Nov 9, 2020

HDembinski commented Nov 9, 2020

matthewfeickert commented Nov 9, 2020 • edited Loading

matthewfeickert commented Nov 9, 2020

matthewfeickert commented Nov 9, 2020

kratsg commented Nov 9, 2020

HDembinski commented Nov 9, 2020

HDembinski commented Nov 9, 2020 • edited Loading

matthewfeickert commented Nov 9, 2020

HDembinski commented Nov 12, 2020

matthewfeickert commented Nov 19, 2020

alexander-held commented Nov 19, 2020

HDembinski commented Nov 19, 2020

matthewfeickert commented Nov 30, 2020

HDembinski commented Dec 7, 2020

HDembinski commented Dec 8, 2020

matthewfeickert commented Dec 11, 2020

matthewfeickert commented Dec 14, 2020

HDembinski commented Dec 14, 2020

matthewfeickert commented Nov 6, 2020 •

edited

Loading

matthewfeickert commented Nov 9, 2020 •

edited

Loading

matthewfeickert commented Nov 9, 2020 •

edited

Loading

HDembinski commented Nov 9, 2020 •

edited

Loading