From d098815717271b83bed88cacad05bb69537d901c Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Sat, 15 Nov 2025 19:15:08 +0100 Subject: [PATCH 1/4] [hist] fix typoes (cherry picked from commit 95fe1d8d2bcb8a25b20beacd3d76022c2fa7020d) --- hist/hist/src/TBackCompFitter.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hist/hist/src/TBackCompFitter.cxx b/hist/hist/src/TBackCompFitter.cxx index b32481ce4981b..4f3d0589af3f1 100644 --- a/hist/hist/src/TBackCompFitter.cxx +++ b/hist/hist/src/TBackCompFitter.cxx @@ -332,7 +332,7 @@ void TBackCompFitter::FixParameter(Int_t ipar) { void TBackCompFitter::GetConfidenceIntervals(Int_t n, Int_t ndim, const Double_t *x, Double_t *ci, Double_t cl) { if (!fFitter->Result().IsValid()) { - Error("GetConfidenceIntervals","Cannot compute confidence intervals with an invalide fit result"); + Error("GetConfidenceIntervals", "Cannot compute confidence intervals with an invalid fit result"); return; } @@ -367,7 +367,7 @@ void TBackCompFitter::GetConfidenceIntervals(Int_t n, Int_t ndim, const Double_t void TBackCompFitter::GetConfidenceIntervals(TObject *obj, Double_t cl) { if (!fFitter->Result().IsValid() ) { - Error("GetConfidenceIntervals","Cannot compute confidence intervals with an invalide fit result"); + Error("GetConfidenceIntervals", "Cannot compute confidence intervals with an invalid fit result"); return; } From 3d440bcd93ad3c59472ccb284558ccbb1c2a6341 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sun, 16 Nov 2025 13:56:07 +0100 Subject: [PATCH 2/4] [RF] Correctly normalize RooSimultaneous when projecting over data Since a8ef8b0, any RooAbsPdf that makes a prediction for the expected number of events will use this prediction for the normalization when plotting. This made is more meaningful to compare models with data because the model normalization was not automatically scaled to match the data. However, it also broke plots with projections of RooSimultaneous pdfs over data, because the temporary RooAddPdf that is created in this case did not use coefficients that were scaled to the actual dataset, but to unity. Correctly normalizing to the sum of entries in the dataset fixes this problem. A unit test is also implemented. Closes #20383. (cherry picked from commit 61735d8ce7c96ee18e9efa83461a25a2e6e7bc8f) --- roofit/roofitcore/src/RooSimultaneous.cxx | 4 +-- roofit/roofitcore/test/CMakeLists.txt | 2 +- .../roofitcore/test/testRooSimultaneous.cxx | 34 +++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/roofit/roofitcore/src/RooSimultaneous.cxx b/roofit/roofitcore/src/RooSimultaneous.cxx index b8805ac0d9b8c..92b3c00b86f62 100644 --- a/roofit/roofitcore/src/RooSimultaneous.cxx +++ b/roofit/roofitcore/src/RooSimultaneous.cxx @@ -810,8 +810,8 @@ RooPlot* RooSimultaneous::plotOn(RooPlot *frame, RooLinkedList& cmdList) const } if (skip) continue ; - // Instantiate a RRV holding this pdfs weight fraction - wgtCompList.addOwned(std::make_unique(proxy->name(),"coef",wTable->getFrac(proxy->name()))); + // Instantiate a RRV holding this pdfs weight + wgtCompList.addOwned(std::make_unique(proxy->name(),"coef",wTable->get(proxy->name()))); sumWeight += wTable->getFrac(proxy->name()) ; // Add the PDF to list list diff --git a/roofit/roofitcore/test/CMakeLists.txt b/roofit/roofitcore/test/CMakeLists.txt index 09d64af90f5ec..029213cde087c 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -65,7 +65,7 @@ ROOT_ADD_GTEST(testRooPolyFunc testRooPolyFunc.cxx LIBRARIES Gpad RooFitCore) ROOT_ADD_GTEST(testRooRealL TestStatistics/testRooRealL.cxx LIBRARIES RooFitCore) ROOT_ADD_GTEST(testRooRombergIntegrator testRooRombergIntegrator.cxx LIBRARIES MathCore RooFitCore) ROOT_ADD_GTEST(testRooSTLRefCountList testRooSTLRefCountList.cxx LIBRARIES RooFitCore) -ROOT_ADD_GTEST(testRooSimultaneous testRooSimultaneous.cxx LIBRARIES RooFitCore) +ROOT_ADD_GTEST(testRooSimultaneous testRooSimultaneous.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooTruthModel testRooTruthModel.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testSumW2Error testSumW2Error.cxx LIBRARIES Gpad RooFitCore) ROOT_ADD_GTEST(testTestStatistics testTestStatistics.cxx LIBRARIES RooFitCore) diff --git a/roofit/roofitcore/test/testRooSimultaneous.cxx b/roofit/roofitcore/test/testRooSimultaneous.cxx index 8bdb760ae2c07..ff58eb7275f90 100644 --- a/roofit/roofitcore/test/testRooSimultaneous.cxx +++ b/roofit/roofitcore/test/testRooSimultaneous.cxx @@ -9,11 +9,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include "gtest_wrapper.h" @@ -507,3 +509,35 @@ TEST_P(TestStatisticTest, RooSimultaneousSingleChannelCrossCheckWithCondVar) EXPECT_TRUE(resSimWrapped->isIdentical(*resDirect)) << "Inconsistency in RooSimultaneous wrapping with ConditionalObservables"; } + +/// GitHub issue #20383. +/// Check that the the simultaneous pdf is normalized correctly when plotting +/// with a projection dataset. +TEST(RooSimultaneous, PlotProjWData) +{ + RooRealVar x("x", "x", -8, 8); + x.setBins(1); + + RooUniform model{"model", "", x}; + RooUniform model_ctl{"model_ctl", "", x}; + + RooCategory sample("sample", "sample", {{"physics", 0}, {"control", 1}}); + + RooArgSet vars{x, sample}; + RooDataHist combData{"combData", "", vars}; + sample.setLabel("physics"); + combData.add(vars, 1000); + sample.setLabel("control"); + combData.add(vars, 2000); + + RooSimultaneous simPdf("simPdf", "simultaneous pdf", {{"physics", &model}, {"control", &model_ctl}}, sample); + + RooPlot *frame = x.frame(); + combData.plotOn(frame); + simPdf.plotOn(frame, RooFit::ProjWData(sample, combData)); + + // The pdf should be normalized to match the data. In this test, we plot a + // single bin and the model is uniform, to the curve should be equal to the + // sum of data entries in the center. + EXPECT_DOUBLE_EQ(frame->getCurve()->interpolate(0.), combData.sumEntries()); +} From e138e3a3617ba143386525956dc1cd7da9f4db29 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sun, 16 Nov 2025 14:34:11 +0100 Subject: [PATCH 3/4] [RF] Avoid warning about duplicate command arguments Avoid warning about duplicate command arguments by removing old arguments when overriding them. (cherry picked from commit d4b150cc83fa6234e128a9945ad9e991badaf845) --- roofit/roofitcore/src/RooAbsPdf.cxx | 11 ++++++++++- roofit/roofitcore/src/RooSimultaneous.cxx | 19 ++++++++++++++----- .../roofit/roofit/rf501_simultaneouspdf.C | 2 +- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/roofit/roofitcore/src/RooAbsPdf.cxx b/roofit/roofitcore/src/RooAbsPdf.cxx index 4e4b050f17769..2ec8d927c8be9 100644 --- a/roofit/roofitcore/src/RooAbsPdf.cxx +++ b/roofit/roofitcore/src/RooAbsPdf.cxx @@ -210,6 +210,15 @@ inline double getLog(double prob, RooAbsReal const *caller) return std::log(prob); } +void replaceOrAdd(RooLinkedList &lst, TObject &obj) +{ + TObject *old = lst.FindObject(obj.GetName()); + if (old) + lst.Replace(old, &obj); + else + lst.Add(&obj); +} + } // namespace using std::endl, std::string, std::ostream, std::vector, std::pair, std::make_pair; @@ -2173,7 +2182,7 @@ RooPlot* RooAbsPdf::plotOn(RooPlot* frame, RooLinkedList& cmdList) const // Append overriding scale factor command at end of original command list RooCmdArg tmp = RooFit::Normalization(scaleFactor,Raw) ; tmp.setInt(1,1) ; // Flag this normalization command as created for internal use (so that VisualizeError can strip it) - cmdList.Add(&tmp) ; + replaceOrAdd(cmdList, tmp); // Was a component selected requested if (haveCompSel) { diff --git a/roofit/roofitcore/src/RooSimultaneous.cxx b/roofit/roofitcore/src/RooSimultaneous.cxx index 92b3c00b86f62..0b241f961c493 100644 --- a/roofit/roofitcore/src/RooSimultaneous.cxx +++ b/roofit/roofitcore/src/RooSimultaneous.cxx @@ -87,6 +87,15 @@ std::map createPdfMap(const RooArgList &inPdfList, Roo return pdfMap; } +void replaceOrAdd(RooLinkedList &lst, TObject &obj) +{ + TObject *old = lst.FindObject(obj.GetName()); + if (old) + lst.Replace(old, &obj); + else + lst.Add(&obj); +} + } // namespace RooSimultaneous::InitializationOutput::~InitializationOutput() = default; @@ -774,9 +783,9 @@ RooPlot* RooSimultaneous::plotOn(RooPlot *frame, RooLinkedList& cmdList) const // WVE -- do not adjust normalization for asymmetry plots RooLinkedList cmdList2(cmdList) ; if (!cmdList.find("Asymmetry")) { - cmdList2.Add(&tmp1) ; + replaceOrAdd(cmdList2, tmp1); } - cmdList2.Add(&tmp2) ; + replaceOrAdd(cmdList2, tmp2); // Plot single component RooPlot* retFrame = getPdf(idxCatClone->getCurrentLabel())->plotOn(frame,cmdList2); @@ -875,15 +884,15 @@ RooPlot* RooSimultaneous::plotOn(RooPlot *frame, RooLinkedList& cmdList) const RooCmdArg tmp2 = RooFit::ProjWData(*projDataSet,*projDataTmp) ; // WVE -- do not adjust normalization for asymmetry plots if (!cmdList.find("Asymmetry")) { - cmdList2.Add(&tmp1) ; + replaceOrAdd(cmdList2, tmp1); } - cmdList2.Add(&tmp2) ; + replaceOrAdd(cmdList2, tmp2); RooPlot* frame2 ; if (!projSetTmp.empty()) { // Plot temporary function RooCmdArg tmp3 = RooFit::Project(projSetTmp) ; - cmdList2.Add(&tmp3) ; + replaceOrAdd(cmdList2, tmp3); frame2 = plotVar.plotOn(frame,cmdList2) ; } else { // Plot temporary function diff --git a/tutorials/roofit/roofit/rf501_simultaneouspdf.C b/tutorials/roofit/roofit/rf501_simultaneouspdf.C index 9668170a4cb95..a658fc66ea844 100644 --- a/tutorials/roofit/roofit/rf501_simultaneouspdf.C +++ b/tutorials/roofit/roofit/rf501_simultaneouspdf.C @@ -92,7 +92,7 @@ void rf501_simultaneouspdf() // --------------------------------------------------- // Perform simultaneous fit of model to data and model_ctl to data_ctl - std::unique_ptr fitResult{simPdf.fitTo(combData, PrintLevel(-1), Save(), PrintLevel(-1))}; + std::unique_ptr fitResult{simPdf.fitTo(combData, Save(), PrintLevel(-1))}; fitResult->Print(); // P l o t m o d e l s l i c e s o n d a t a s l i c e s From 8c5b1ff170ff68b6296dc77460bdd56095e7c312 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Sun, 16 Nov 2025 14:46:57 +0100 Subject: [PATCH 4/4] [RF] Return reference to actual RooAbsReal in `RooPolyVar::x()` getter Just like in any other getter for the inputs of RooFit functions, the `RooPolyVar::x()` getter should return the underlying RooAbsReal, instead of the internal proxy. Closes #20293. (cherry picked from commit 66cd6aeb6b4ced8f368331a17661d9613e1de17c) --- roofit/roofitcore/inc/RooPolyVar.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roofit/roofitcore/inc/RooPolyVar.h b/roofit/roofitcore/inc/RooPolyVar.h index ff3bd3d88c7d0..4957a573f9693 100644 --- a/roofit/roofitcore/inc/RooPolyVar.h +++ b/roofit/roofitcore/inc/RooPolyVar.h @@ -34,7 +34,7 @@ class RooPolyVar : public RooAbsReal { Int_t getAnalyticalIntegral(RooArgSet &allVars, RooArgSet &analVars, const char *rangeName = nullptr) const override; double analyticalIntegral(Int_t code, const char *rangeName = nullptr) const override; - RooRealProxy const &x() const { return _x; } + RooAbsReal const &x() const { return _x.arg(); } RooArgList const &coefList() const { return _coefList; } int lowestOrder() const { return _lowestOrder; }