Skip to content
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

Remove Proof from HybridNew, as it was removed from RooStats #1033

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

Conversation

gpetruc
Copy link
Contributor

@gpetruc gpetruc commented Dec 11, 2024

Removing support for Proof in HybridNew calculator, as it was removed from RooFit and RooStats in root-project/root#14993 , so that the code still compiles on the ROOT master branch (tested on the dev3 LCG nightly build)
I don't think anyone ever used in the past ten years (maybe not even before), and from the ROOT PR my understanding is that it was anyway not functional since several root releases.

@guitargeek

@guitargeek
Copy link
Contributor

Thanks for making these adjustments! Proof needs so serialize the likelihood with ROOT IO to send it to the different workers, and I disabled ROOT IO for the likelihood classes already in ROOT 6.28, accidentally breaking PROOF support in RooFit:

As nobody has complained about that since then, I think we can safely conclude that it was unused.

If we need parallelization to speed up something in RooFit (like the generation of toys), it should be re-implemented with modern C++ approaches like std::thread or what RDataFrame does (building on top of TBB).

So: LGTM for this PR from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants