[Minuit2] Cache transformed parameter values in MnHesse - #17817
Conversation
Test Results 18 files 18 suites 4d 7h 21m 11s ⏱️ Results for commit 8c66f2a. ♻️ This comment has been updated with latest results. |
b4af712 to
9916ea2
Compare
There was a problem hiding this comment.
Thank Jonas for the PR. It is great you have found this and achieved this speedup!
Can you maybe split the PR in 2, one fixing the MnHesse, the other applying other changes in the code (renaming files, etc....)?
The second one can then applied later after we merged the pending PR in Minuit2
|
Hi @lmoneta, of course you're right. I was abusing this PR as a development branch to get CI feedback, I'll remove all the commits unrelated to the functional change in MnHesse and create a different PR from that. |
Some functions that were already mentioned as "internal" in the docs are removed from the public interface.
56be3bb to
8c66f2a
Compare
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM!
Thank you Jonas for this optimisation. I have just a small comment on the naming of the data members for the new class caching the internal parameter values
It was figured out with `perf` and `flamegraph.pl` that the main performance bottleneck when using Minuits Hesse on RooFit likelihoods is the transformation to internal Minuit parameters in MnHesse, which is a relatively expensive trigonometric operation. It is done for all parameters at every function operation, even if only one parameter is changed. We need to do order n-squared function calls for the Hessian. The total runtime of calling the RooFit function itself scales only linearly with the number of parameters, thanks to the caching in RooFit. However, the parameter transformation in MnHesse implements no caching and therefore has quadratic cost. This commit implements caching for the transformed parameters to make this bottleneck go away completely. With the changes in this commit, the plan-of-work item of "Speedup the computation of the Hessian for big Higgs combinations at least by factor of 2" is completed. Our ATLAS benchmark is now doing the Hesse step in 100 s instead of 120 s. But as the addressed bottleneck grows with the number of fit parameters squared, this optimization will have a much stronger impact on some reported user workflows, where computing the Hessian takes hours right now. In any case, considering also the performance improvements in other commits in this development cycle, one gets a 2 x speedup in our benchmark too. This change in Minuit indirectly affects all RooFit users. I saw that this parameter transformation is also the main bottleneck in evaluating Hessians with likelihoods from CMS combine.
2 participants
It was figured out with
perfandflamegraph.plthat the mainperformance bottleneck when using Minuits Hesse on RooFit likelihoods is
the transformation to internal Minuit parameters in MnHesse, which is a
relatively expensive trigonometric operation. It is done for all
parameters at every function operation, even if only one parameter is
changed. We need to do order n-squared function calls for the Hessian.
The total runtime of calling the RooFit function itself scales only
linearly with the number of parameters, thanks to the caching in RooFit.
However, the parameter transformation in MnHesse implements no caching
and therefore has quadratic cost.
This PR implements caching for the transformed parameters to make
this bottleneck go away completely.
With the changes in this PR, the plan-of-work item of "Speedup the
computation of the Hessian for big Higgs combinations at least by factor
of 2" is completed. Our ATLAS benchmark is now doing the Hesse step in
100 s instead of 120 s. But as the addressed bottleneck grows with the
number of fit parameters squared, this optimization will have a much
stronger impact on some reported user workflows, where computing the
Hessian takes hours right now. In any case, considering also the
performance improvements in other PRs in this development cycle, one
gets a 2 x speedup in our benchmark too (see #17816).
This change in Minuit indirectly affects all RooFit users. I saw that
this parameter transformation is also the main bottleneck in evaluating
Hessians with likelihoods from CMS combine.
ATLAS Higgs combination benchmark
With ROOT master
Total runtime of
minimize()andhesse(): 160 s.With this PR and #17816
Total runtime of
minimize()andhesse(): 105 s (34 % faster).The new bottlenecks are again in RooFit:
RooAbsArg::setValueDirty()(about 10 s runtime, we can get rid of it easily because the new CPU evaluation backend doesn't use the dirty flag information anyway)RooFit::Evaluator::run()that is not related to actual computation, again 10 more seconds. I don't know what to do about it.In particular, the
setValueDirty()is responsible for most of the runtime in the line search. If we get rid of it, the line search will bottleneck fits with AD much less, where the gradient step is very fast and the line search is the bottleneck of the overall minimization.