Skip to content

[RF] Faster Hesse in RooFit by advertising which params are independent #16394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Sep 9, 2024

This reduces the time to run Hesse in the ATLAS Higgs benchmark from
123 s to 92 seconds.

Given that some models take hours for this, this is a significant
improvement for the user experience.

Further improvement is possible by analyzing the computation graph a bit
more to find more independent parameters (e.g., the different gammas for
stat uncertainties from different bins).

Benchmark results

The Higgs combination benchmark takes 31 s to minimize, plus 79 s for the Hessian with ROOT master. It was 130 seconds with ROOT master just a few days ago, before some optimization PRs got merged. This PR reduces the time for hesse() to 57 seconds.

Copy link

github-actions bot commented Sep 9, 2024

Test Results

    18 files      18 suites   4d 11h 12m 15s ⏱️
 2 725 tests  2 724 ✅ 0 💤 1 ❌
47 351 runs  47 350 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 3fa8122.

♻️ This comment has been updated with latest results.

@dpiparo
Copy link
Member

dpiparo commented Sep 14, 2024

The error seems an unresolved symbol on Win

2024-09-14T07:01:53.2104016Z      Creating library Z:/foobar/lib/libRooFitCore.lib and object Z:/foobar/lib/libRooFitCore.exp
2024-09-14T07:01:53.4308777Z RooXYChi2Var.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl RooAbsArg::fillVariableGroups(class RooFit::VariableGroups &)const " (?fillVariableGroups@RooAbsArg@@UEBAXAEAVVariableGroups@RooFit@@@Z) [Z:\foobar\build\roofit\roofitcore\RooFitCore.vcxproj]
2024-09-14T07:01:53.4311585Z RooAbsTestStatistic.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl RooAbsArg::fillVariableGroups(class RooFit::VariableGroups &)const " (?fillVariableGroups@RooAbsArg@@UEBAXAEAVVariableGroups@RooFit@@@Z) [Z:\foobar\build\roofit\roofitcore\RooFitCore.vcxproj]

@guitargeek guitargeek force-pushed the hessian_optimization branch 2 times, most recently from 5a2d19a to 04afc01 Compare September 14, 2024 15:38
@dpiparo dpiparo self-requested a review September 15, 2024 08:16
Copy link
Member

@dpiparo dpiparo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for this improvement! Let's not forget to give to this feature the right emphasis in the RNs.

@guitargeek guitargeek force-pushed the hessian_optimization branch from 04afc01 to 9ebb89a Compare October 28, 2024 13:59
@guitargeek guitargeek force-pushed the hessian_optimization branch 2 times, most recently from f3f761d to 5d5d765 Compare February 15, 2025 12:31
This reduces the time to run Hesse in the ATLAS Higgs benchmark from
123 s to 92 seconds.

Given that some models take hours for this, this is a significant
improvement for the user experience.

Further improvement is possible by analyzing the computation graph a bit
more to find more independent parameters (e.g., the different gammas for
stat uncertainties from different bins).
@guitargeek guitargeek force-pushed the hessian_optimization branch from 82825d7 to 3fa8122 Compare March 11, 2025 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants