For #11, I extended the AnalyticalGradient class (and all necessary dependent classes) with external Hessians (g2) and step sizes. However, the extension is incomplete. Most importantly, I did not check the correctness of the transformations from external to internal parameter space of the Hessian and step size. In fact, the current step size transformation simply multiplies by 1.
It turns out that doing external gradients is not easy if you want Minuit2 to still perform well; the boundary handling as Minuit2 implements it is a crucial aspect in its performance and so I completely copied this behavior into the RooFit gradient routines. This, in turn, meant I did not have to figure out the exact correct transformations from and to Minuit2, because everything in my RooFit implementation was already in Minuit2-internal parameter space.
To accomodate for this use-case in a cleaner way, we could build a InternalParameterSpaceExternalGradient class (input on better name more than welcome) that expects the parameters, gradients, etc. to be in Minuit2-internal space. This would save a lot of going back and forth, but will still allow us to handle the gradient algorithm externally. A new class seems like a cleaner option than changing the existing class, since this is really a conceptually different approach than the intended use-case for AnalyticalGradient. The AnalyticalGradient can then be completed by adding the correct transformations for Hessian and step size and can potentially be used in the future by other users.
So, in summary, I think the following should be done:
[ ] Build InternalParameterSpaceExternalGradient class (and corresponding MnFunction types etc.)
[ ] Finish AnalyticalGradient handling of external Hessians and step sizes (as started in #11), by doing correct transformations.
For #11, I extended the AnalyticalGradient class (and all necessary dependent classes) with external Hessians (
g2) and step sizes. However, the extension is incomplete. Most importantly, I did not check the correctness of the transformations from external to internal parameter space of the Hessian and step size. In fact, the current step size transformation simply multiplies by 1.It turns out that doing external gradients is not easy if you want Minuit2 to still perform well; the boundary handling as Minuit2 implements it is a crucial aspect in its performance and so I completely copied this behavior into the RooFit gradient routines. This, in turn, meant I did not have to figure out the exact correct transformations from and to Minuit2, because everything in my RooFit implementation was already in Minuit2-internal parameter space.
To accomodate for this use-case in a cleaner way, we could build a InternalParameterSpaceExternalGradient class (input on better name more than welcome) that expects the parameters, gradients, etc. to be in Minuit2-internal space. This would save a lot of going back and forth, but will still allow us to handle the gradient algorithm externally. A new class seems like a cleaner option than changing the existing class, since this is really a conceptually different approach than the intended use-case for AnalyticalGradient. The AnalyticalGradient can then be completed by adding the correct transformations for Hessian and step size and can potentially be used in the future by other users.
So, in summary, I think the following should be done:
@lmoneta, @wverkerke, what do you think?