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

[RF] Using a conditional RooProdPdf in a multi-channel fit spawns too many integrals with new CPU evaluation backend #15751

Closed
guitargeek opened this issue Jun 5, 2024 · 0 comments · Fixed by #15795

Comments

@guitargeek
Copy link
Contributor

guitargeek commented Jun 5, 2024

Description

Using a conditional RooProdPdf in a RooSimultaneous spawns too many integrals with new CPU evaluation backend.

This is a deal breaker for fitting the open likelihood of the CMS Run 1 Higgs analysis with the new CPU evaluation backend, because evaluating these unnecessary integrals takes too much time (although the result is correct).

Since the RooFit AD backend uses the same "compileForNormSet" method to sanitize the computation graph, it's also a blocker for minimizing the CMS Higgs Run 1 likelihood with AD.

Reproducer

RooRealVar x{"x", "x", 0, 1};
RooRealVar y{"y", "y", 0, 1};

RooGenericPdf pdfx{"pdfx", "1.0 + x - x", {x}};
RooGenericPdf pdfxy{"pdfxy", "1.0 + x - x + y - y", {x, y}};

RooProdPdf pdf{"pdf", "pdf", pdfx, RooFit::Conditional(pdfxy, y)};

RooArgSet normSet{x, y};

RooCategory cat{"cat", "cat", {{"0", 0}}};
RooSimultaneous simPdf{"simPdf", "simPdf", {{"0", &pdf}}, cat};

{
   std::cout << "This looks good:" << std::endl;
   RooFit::Detail::CompileContext ctx{normSet};
   std::unique_ptr<RooAbsPdf> compiled{static_cast<RooAbsPdf*>(pdf.compileForNormSet(normSet, ctx).release())};
   compiled->getVal(normSet);
}
{
   std::cout << "Too many integrals:" << std::endl;
   RooFit::Detail::CompileContext ctx{normSet};
   std::unique_ptr<RooAbsPdf> compiled{static_cast<RooAbsPdf*>(simPdf.compileForNormSet(normSet, ctx).release())};
   compiled->getVal(normSet);
}

The output is:

This looks good:
[#1] INFO:NumericIntegration -- RooRealIntegral::init(pdfx_Int[x]) using numeric integrator RooIntegrator1D to calculate Int(x)
[#1] INFO:NumericIntegration -- RooRealIntegral::init(pdfxy_Int[y]) using numeric integrator RooIntegrator1D to calculate Int(y)

Too many integrals:
[#1] INFO:NumericIntegration -- RooRealIntegral::init(_0_pdfx_Int[_0_x]) using numeric integrator RooIntegrator1D to calculate Int(_0_x)
[#1] INFO:NumericIntegration -- RooRealIntegral::init(_0_pdfxy_Int[_0_y]) using numeric integrator RooIntegrator1D to calculate Int(_0_y)
[#1] INFO:NumericIntegration -- RooRealIntegral::init([_0_pdfx_over__0_pdfx_Int[_0_x]_X__0_pdfxy_over__0_pdfxy_Int[_0_y]]_Norm[_0_x,_0_y]_denominator_Int[_0_x,_0_y]) using numeric integrator RooAdaptiveIntegratorND to calculate Int(_0_x,_0_y)
[#1] INFO:NumericIntegration -- RooRealIntegral::init(_0_pdfx_Int[_0_x]) using numeric integrator RooIntegrator1D to calculate Int(_0_x)
[#1] INFO:NumericIntegration -- RooRealIntegral::init(_0_pdfxy_Int[_0_y]) using numeric integrator RooIntegrator1D to calculate Int(_0_y)
[#1] INFO:NumericIntegration -- RooRealIntegral::init(_0_pdfx_Int[_0_x]) using numeric integrator RooIntegrator1D to calculate Int(_0_x)
@guitargeek guitargeek self-assigned this Jun 5, 2024
@guitargeek guitargeek changed the title [RF] Using a conditional RooProdPdf in a RooSimultaneous spawns too many integrals with new CPU evaluation backend [RF] Using a conditional RooProdPdf in a multi-channel fit spawns too many integrals with new CPU evaluation backend Jun 5, 2024
guitargeek added a commit to guitargeek/root that referenced this issue Jun 7, 2024
When compiling a RooSimultaneous for a given normalization set, we can
only prefix the observables after everything related the compiling of
the compute graph for the normalization set is done. This is because of
a subtlety in conditional RooProdPdfs, which stores the normalization
sets for the individual pdfs in RooArgSets that are disconnected from
the computation graph, so we have no control over them. An alternative
would be to use recursive server re-direction, but this has more
performance overhead.

Closes root-project#15751 and fixes the CMS Run 1 Higgs combination with the new
NLL evaluation backend.
guitargeek added a commit that referenced this issue Jun 8, 2024
When compiling a RooSimultaneous for a given normalization set, we can
only prefix the observables after everything related the compiling of
the compute graph for the normalization set is done. This is because of
a subtlety in conditional RooProdPdfs, which stores the normalization
sets for the individual pdfs in RooArgSets that are disconnected from
the computation graph, so we have no control over them. An alternative
would be to use recursive server re-direction, but this has more
performance overhead.

Closes #15751 and fixes the CMS Run 1 Higgs combination with the new
NLL evaluation backend.
guitargeek added a commit to guitargeek/root that referenced this issue Jun 10, 2024
When compiling a RooSimultaneous for a given normalization set, we can
only prefix the observables after everything related the compiling of
the compute graph for the normalization set is done. This is because of
a subtlety in conditional RooProdPdfs, which stores the normalization
sets for the individual pdfs in RooArgSets that are disconnected from
the computation graph, so we have no control over them. An alternative
would be to use recursive server re-direction, but this has more
performance overhead.

Closes root-project#15751 and fixes the CMS Run 1 Higgs combination with the new
NLL evaluation backend.
guitargeek added a commit that referenced this issue Jun 10, 2024
When compiling a RooSimultaneous for a given normalization set, we can
only prefix the observables after everything related the compiling of
the compute graph for the normalization set is done. This is because of
a subtlety in conditional RooProdPdfs, which stores the normalization
sets for the individual pdfs in RooArgSets that are disconnected from
the computation graph, so we have no control over them. An alternative
would be to use recursive server re-direction, but this has more
performance overhead.

Closes #15751 and fixes the CMS Run 1 Higgs combination with the new
NLL evaluation backend.
silverweed pushed a commit to silverweed/root that referenced this issue Aug 19, 2024
When compiling a RooSimultaneous for a given normalization set, we can
only prefix the observables after everything related the compiling of
the compute graph for the normalization set is done. This is because of
a subtlety in conditional RooProdPdfs, which stores the normalization
sets for the individual pdfs in RooArgSets that are disconnected from
the computation graph, so we have no control over them. An alternative
would be to use recursive server re-direction, but this has more
performance overhead.

Closes root-project#15751 and fixes the CMS Run 1 Higgs combination with the new
NLL evaluation backend.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant