From 7369eec20800e3554d71d7456288520dfb3780be Mon Sep 17 00:00:00 2001 From: "will.buttinger" Date: Tue, 12 May 2026 14:11:36 +0100 Subject: [PATCH 1/5] Update xRooFit --- .../xroofit/inc/RooFit/xRooFit/xRooNLLVar.h | 4 +- roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h | 22 ++- roofit/xroofit/src/xRooFit.cxx | 77 +++++--- roofit/xroofit/src/xRooFitVersion.h | 4 +- roofit/xroofit/src/xRooHypoSpace.cxx | 45 +++-- roofit/xroofit/src/xRooNLLVar.cxx | 59 ++++-- roofit/xroofit/src/xRooNode.cxx | 176 ++++++++++++------ 7 files changed, 279 insertions(+), 108 deletions(-) diff --git a/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h b/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h index 9887bb4797231..bd348cef9ff44 100644 --- a/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h +++ b/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h @@ -405,8 +405,8 @@ class xRooNLLVar : public std::shared_ptr { double alt_value = std::numeric_limits::quiet_NaN(), const xRooFit::Asymptotics::PLLType &pllType = xRooFit::Asymptotics::Unknown); xRooHypoSpace hypoSpace(const char *parName, xRooFit::TestStatistic::Type tsType, int nPoints = 0, - double low = -std::numeric_limits::infinity(), - double high = std::numeric_limits::infinity(), + double low = std::numeric_limits::quiet_NaN(), + double high = std::numeric_limits::quiet_NaN(), double alt_value = std::numeric_limits::quiet_NaN()) { return hypoSpace(parName, nPoints, low, high, alt_value, xRooFit::Asymptotics::Unknown, tsType); diff --git a/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h b/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h index df9f877b2493e..6e00395de831d 100644 --- a/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h +++ b/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h @@ -197,7 +197,23 @@ class xRooNode : public TNamed, public std::vector> { return aa == bb; }; }; - auto begin() const -> xRooNodeIterator { return xRooNodeIterator(std::vector>::begin()); } + auto begin() const -> xRooNodeIterator + { + // this update is to resolve need for calling browse() before iterating, e.g. + // for(auto a : xRooNode(b).browse()) ... + // can now become the more natural: + // for(auto a : xRooNode(b)) ... + // but would still need e.g. xRooNode(s).browse().size() rather than xRooNode(s).size() which would give 0 for + // unpopulated case + static bool browseLock = + false; // need for blocking recursive calls to browse (since browse method uses the iterators) + if (!browseLock && get() && empty()) { + browseLock = true; + const_cast(*this).browse(); + browseLock = false; + } + return xRooNodeIterator(std::vector>::begin()); + } auto end() const -> xRooNodeIterator { return xRooNodeIterator(std::vector>::end()); } // needed in pyROOT to avoid it creating iterators that follow the 'get' to death @@ -312,6 +328,9 @@ class xRooNode : public TNamed, public std::vector> { xRooNode datasets() const; // datasets corresponding to this pdf (parent nodes that do observable selections automatically applied) + xRooNode parents() const; // the clients of this node + xRooNode args() const; // all the nodes underneath this node + xRooNode Replace(const xRooNode &node); // use to replace a node in the tree at the location of this node xRooNode Remove(const xRooNode &child); xRooNode @@ -326,6 +345,7 @@ class xRooNode : public TNamed, public std::vector> { xRooNode reduced(const std::string &range = "", bool invert = false) const; // return a node representing reduced version of this node, will use the SetRange to reduce if blank + xRooNode reduced(const std::function selector) const; // following versions are for the menu in the GUI /** @private */ diff --git a/roofit/xroofit/src/xRooFit.cxx b/roofit/xroofit/src/xRooFit.cxx index 9288df5a17795..440eca74bbc13 100644 --- a/roofit/xroofit/src/xRooFit.cxx +++ b/roofit/xroofit/src/xRooFit.cxx @@ -64,6 +64,8 @@ #include "TROOT.h" #include "TBrowser.h" +#include "Python.h" + BEGIN_XROOFIT_NAMESPACE std::shared_ptr xRooFit::sDefaultNLLOptions = nullptr; @@ -222,9 +224,8 @@ xRooFit::generateFrom(RooAbsPdf &pdf, const RooFitResult &_fr, bool expected, in !(cClass && strcmp(cClass->GetName(), "SimpleGaussianConstraint") == 0)) { TString className = (cClass) ? cClass->GetName() : "undefined"; oocoutW((TObject *)nullptr, Generation) - << "xRooFit::generateFrom : constraint term " << thePdf->GetName() - << " of type " << className << " is a non-supported type - result might be not correct " - << std::endl; + << "xRooFit::generateFrom : constraint term " << thePdf->GetName() << " of type " + << className << " is a non-supported type - result might be not correct " << std::endl; } // in case of a Poisson constraint make sure the rounding is not set @@ -514,7 +515,9 @@ std::shared_ptr xRooFit::defaultFitConfig() extraOpts->SetValue("OptimizeConst", 2); // if 0 will disable constant term optimization and cache-and-track of the // NLL. 1 = just caching, 2 = cache and track #if ROOT_VERSION_CODE >= ROOT_VERSION(6, 29, 00) - extraOpts->SetValue("StrategySequence", "0s01s12s2s3m"); + extraOpts->SetValue( + "StrategySequence", + "0s01s12s2s3"); // 'm' would indicate use migradImproved from minuit v1. Dropped from default for 6.40 extraOpts->SetValue("HesseStrategySequence", "23"); #else extraOpts->SetValue("StrategySequence", "0s01s12s2m"); @@ -541,6 +544,29 @@ ROOT::Math::IOptions *xRooFit::defaultFitConfigOptions() return const_cast(defaultFitConfig()->MinimizerOptions().ExtraOptions()); } +void printCerr(const char *msg) +{ + if (Py_IsInitialized()) { + PySys_WriteStderr("%s\n", msg); + if (PyObject *sys_stdout = PySys_GetObject("stderr"); sys_stdout != nullptr) { + Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); + } + } else { + std::cerr << msg << std::endl; + } +} +void printCout(const char *msg) +{ + if (Py_IsInitialized()) { + PySys_WriteStdout("%s\n", msg); + if (PyObject *sys_stdout = PySys_GetObject("stdout"); sys_stdout != nullptr) { + Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); + } + } else { + std::cout << msg << std::endl; + } +} + class ProgressMonitor : public RooAbsReal { public: void (*oldHandlerr)(int) = nullptr; @@ -549,7 +575,7 @@ class ProgressMonitor : public RooAbsReal { static void interruptHandler(int signum) { if (signum == SIGINT) { - std::cout << "Minimization interrupted ... will exit as soon as possible" << std::endl; + printCout("Minimization interrupted ... will exit as soon as possible"); // TODO: create a global mutex for this fInterrupt = true; } else { @@ -625,14 +651,14 @@ class ProgressMonitor : public RooAbsReal { s.Reset(); std::stringstream sout; - sout << (counter) << ") (" << evalRate << "Hz) " << TDatime().AsString(); + sout << TDatime().AsString() << ":(" << (counter) << "|" << evalRate << "Hz)"; if (!fState.empty()) sout << " : " << fState; if (counter2) { // doing a hesse step, estimate progress based on evaluations int nRequired = prevPars.size(); if (nRequired > 1) { - nRequired *= (nRequired-1); + nRequired *= (nRequired - 1); nRequired /= 2; // since only need to do the a 'triangle' of the hessian matrix if (fState == "Hesse3") { nRequired *= 4; @@ -640,7 +666,7 @@ class ProgressMonitor : public RooAbsReal { sout << " (~" << int(100.0 * (counter - counter2) / nRequired) << "%)"; } } - sout << " : " << minVal << " Delta = " << (minVal - prevMin); + sout << " : " << minVal << " Delta=" << (minVal - prevMin); if (minVal < prevMin) { sout << " : "; // compare minPars and prevPars, print biggest deltas @@ -685,7 +711,7 @@ class ProgressMonitor : public RooAbsReal { } gSystem->ProcessEvents(); } - std::cerr << sout.str() << std::endl; + printCerr(sout.str().c_str()); // std::cerr << sout.str() << std::endl; prevMin = minVal; prevCounter = counter; @@ -699,10 +725,11 @@ class ProgressMonitor : public RooAbsReal { mutable int counter = 0; int counter2 = 0; // used to estimate progress of a Hesse calculation -private: - RooRealProxy fFunc; mutable double minVal = std::numeric_limits::infinity(); mutable double prevMin = std::numeric_limits::infinity(); + +private: + RooRealProxy fFunc; mutable RooArgList minPars; mutable RooArgList prevPars; mutable int prevCounter = 0; @@ -1094,11 +1121,13 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, // specified Also note that if fits are failing because of edm over max, it can be a good idea to activate the // Offset option when building nll if (printLevel >= -1) { - Warning("fitTo", "%s %s%s Status=%d (edm=%f, tol=%f, strat=%d), tries=#%d...", fitName.Data(), - _minimizer.fitter()->Config().MinimizerType().c_str(), - _minimizer.fitter()->Config().MinimizerAlgoType().c_str(), status, - _minimizer.fitter()->Result().Edm(), _minimizer.fitter()->Config().MinimizerOptions().Tolerance(), - _minimizer.fitter()->Config().MinimizerOptions().Strategy(), tries); + printCerr(TString::Format("Warning: %s %s%s Status=%d (edm=%f, tol=%f, strat=%d), tries=#%d...", + fitName.Data(), _minimizer.fitter()->Config().MinimizerType().c_str(), + _minimizer.fitter()->Config().MinimizerAlgoType().c_str(), status, + _minimizer.fitter()->Result().Edm(), + _minimizer.fitter()->Config().MinimizerOptions().Tolerance(), + _minimizer.fitter()->Config().MinimizerOptions().Strategy(), tries) + .Data()); } // decide what to do next based on strategy sequence @@ -1196,6 +1225,8 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, if (auto fff = dynamic_cast(_nll); fff) { fff->fState = TString::Format("Hesse%d", _minimizer.fitter()->Config().MinimizerOptions().Strategy()); fff->counter2 = fff->counter; + fff->prevMin = + fff->minVal; // reset minimum when change to hesse. Helps see if hesse eval gives new lower values } //_nll->getVal(); // for reasons I dont understand, if nll evaluated before hesse call the edm is smaller? - @@ -1235,8 +1266,9 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, } if ((_status != 0 || _minimizer.fitter()->GetMinimizer()->CovMatrixStatus() != 3) && status == 0 && printLevel >= -1) { - Warning("fitTo", "%s hesse status is %d, covQual=%d", fitName.Data(), _status, - _minimizer.fitter()->GetMinimizer()->CovMatrixStatus()); + printCerr(TString::Format("Warning: %s hesse status is %d, covQual=%d", fitName.Data(), _status, + _minimizer.fitter()->GetMinimizer()->CovMatrixStatus()) + .Data()); } if (sIdx >= m_hessestrategy.Length() - 1) { @@ -1260,6 +1292,7 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, if (auto fff = dynamic_cast(_nll); fff) { fff->fState = "Minos"; fff->counter2 = 0; + fff->prevMin = fff->minVal; } auto _status = _minimizer.minos(*mpars); statusHistory.push_back(std::pair("Minos", _status)); @@ -1288,9 +1321,11 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, out->edm() > _minimizer.fitter()->Config().MinimizerOptions().Tolerance() * 1e-3 && out->status() != 3) { // hesse may have updated edm by using a better strategy than used in the minimization // so print a warning about this - std::cerr << "Warning: post-Hesse edm " << out->edm() << " greater than allowed by tolerance " - << _minimizer.fitter()->Config().MinimizerOptions().Tolerance() * 1e-3 - << ", consider increasing minimization strategy" << std::endl; + std::stringstream ss; + ss << "Warning: post-Hesse edm " << out->edm() + << " > tolerance max edm:" << _minimizer.fitter()->Config().MinimizerOptions().Tolerance() * 1e-3 + << ". Consider increasing your minimization strategy"; + printCerr(ss.str().c_str()); // Dec24: As this is a new warning, will not update status code for now, so edm will be large // but in the future we should probably update the code to 3 so that users don't miss this warning. // out->setStatus(3); // edm above max diff --git a/roofit/xroofit/src/xRooFitVersion.h b/roofit/xroofit/src/xRooFitVersion.h index cf46130bb4d64..c7c71e2f21c76 100644 --- a/roofit/xroofit/src/xRooFitVersion.h +++ b/roofit/xroofit/src/xRooFitVersion.h @@ -12,5 +12,5 @@ #pragma once -#define GIT_COMMIT_HASH "v0.0.2" -#define GIT_COMMIT_DATE "2026-04-28 15:01:00 +0100" +#define GIT_COMMIT_HASH "v0.0.3" +#define GIT_COMMIT_DATE "2026-05-12 14:07:11 +0100" diff --git a/roofit/xroofit/src/xRooHypoSpace.cxx b/roofit/xroofit/src/xRooHypoSpace.cxx index f016ee2ac791f..0f13ac3d6e896 100644 --- a/roofit/xroofit/src/xRooHypoSpace.cxx +++ b/roofit/xroofit/src/xRooHypoSpace.cxx @@ -37,6 +37,8 @@ #include "RooStats/HypoTestInverterResult.h" #include "TEnv.h" +#include "Python.h" + BEGIN_XROOFIT_NAMESPACE xRooNLLVar::xRooHypoSpace::xRooHypoSpace(const char *name, const char *title) @@ -316,16 +318,14 @@ int xRooNLLVar::xRooHypoSpace::scan(const char *type, size_t nPoints, double low } if (/*high < low ||*/ (high == low && nPoints != 1)) { - // take from parameter + // take from parameter (will be either the defaults from the construction of the hypoSpace or whatever last scan + // was low = p->getMin("scan"); high = p->getMax("scan"); } if (!std::isnan(low) && !std::isnan(high) && !(std::isinf(low) && std::isinf(high))) { p->setRange("scan", std::min(low, high), std::max(low, high)); } - if (p->hasRange("scan")) { - ::Info("xRooHypoSpace::scan", "Using %s scan range: %g - %g", p->GetName(), p->getMin("scan"), p->getMax("scan")); - } bool doObs = false; for (auto nSigma : nSigmas) { @@ -403,6 +403,10 @@ int xRooNLLVar::xRooHypoSpace::scan(const char *type, size_t nPoints, double low if (nPoints == 0) { // automatic scan if (sType.Contains("cls")) { + if (p->hasRange("scan")) { + ::Info("xRooHypoSpace::scan", "Using %s scan range: %g - %g", p->GetName(), p->getMin("scan"), + p->getMax("scan")); + } for (double nSigma : nSigmas) { xValueWithError res(std::make_pair(0., 0.)); if (std::isnan(nSigma)) { @@ -428,8 +432,8 @@ int xRooNLLVar::xRooHypoSpace::scan(const char *type, size_t nPoints, double low out += 1; } else { // fit was ok, so use the values to determine an appropriate range - low = std::max(low, back().mu_hat().getVal()-back().mu_hat().getError()*3); - high = std::min(high, back().mu_hat().getVal()+back().mu_hat().getError()*3); + low = std::max(low, back().mu_hat().getVal() - back().mu_hat().getError() * 3); + high = std::min(high, back().mu_hat().getVal() + back().mu_hat().getError() * 3); nPoints = 20; double step = (high - low) / (nPoints - 1); for (size_t i = 0; i < nPoints; i++) { @@ -438,7 +442,6 @@ int xRooNLLVar::xRooHypoSpace::scan(const char *type, size_t nPoints, double low if (back().status() != 0) out += 1; } - } } else { throw std::runtime_error(TString::Format("Automatic scanning not yet supported for %s", type)); @@ -642,7 +645,15 @@ xRooNLLVar::xRooHypoPoint &xRooNLLVar::xRooHypoSpace::AddPoint(const char *coord } coordString.erase(coordString.end() - 1); - ::Info("xRooHypoSpace::AddPoint", "Added new point @ %s", coordString.c_str()); + if (Py_IsInitialized()) { + auto s = TString::Format("Info in : Added new point @ %s", coordString.c_str()); + PySys_WriteStdout("%s\n", s.Data()); + if (PyObject *sys_stdout = PySys_GetObject("stdout"); sys_stdout != nullptr) { + Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); + } + } else { + ::Info("xRooHypoSpace::AddPoint", "Added new point @ %s", coordString.c_str()); + } return emplace_back(out); } @@ -1298,7 +1309,7 @@ xRooNLLVar::xRooHypoSpace::findlimit(const char *opt, double relUncert, unsigned if (!gr || gr->GetN() < 1) { if (maxTries == 0 || std::isnan(AddPoint(TString::Format("%s=%g", v->GetName(), muMin)).getVal(sOpt).first)) { // first point failed ... give up - ::Error("findlimit", "Problem evaluating %s @ %s=%g", sOpt.Data(), v->GetName(), muMin); + ::Error("findlimit", "Problem evaluating First Point %s @ %s=%g", sOpt.Data(), v->GetName(), muMin); return std::pair(std::numeric_limits::quiet_NaN(), 0.); } gr.reset(); @@ -1334,7 +1345,8 @@ xRooNLLVar::xRooHypoSpace::findlimit(const char *opt, double relUncert, unsigned if (maxTries == 0 || std::isnan(AddPoint(TString::Format("%s=%g", v->GetName(), nextPoint)).getVal(sOpt).first)) { // second point failed ... give up - ::Error("xRooHypoSpace::findlimit", "Problem evaluating %s @ %s=%g", sOpt.Data(), v->GetName(), nextPoint); + ::Error("xRooHypoSpace::findlimit", "Problem evaluating Second Point %s @ %s=%g", sOpt.Data(), v->GetName(), + nextPoint); return std::pair(std::numeric_limits::quiet_NaN(), 0.); } gr.reset(); @@ -1396,14 +1408,15 @@ xRooNLLVar::xRooHypoSpace::findlimit(const char *opt, double relUncert, unsigned // got here need a new point .... evaluate the estimated lim location +/- the relUncert (signed error takes care of // direction) + if (maxTries == 0) { + ::Warning("xRooHypoSpace::findlimit", "Reached max number of point evaluations"); + return lim; + } + ::Info("xRooHypoSpace::findlimit", "%s -- Testing new point @ %s=%g (delta=%g)", sOpt.Data(), v->GetName(), nextPoint, lim.second); - if (maxTries == 0 || std::isnan(AddPoint(TString::Format("%s=%g", v->GetName(), nextPoint)).getVal(sOpt).first)) { - if (maxTries == 0) { - ::Warning("xRooHypoSpace::findlimit", "Reached max number of point evaluations"); - } else { - ::Error("xRooHypoSpace::findlimit", "Problem evaluating %s @ %s=%g", sOpt.Data(), v->GetName(), nextPoint); - } + if (std::isnan(AddPoint(TString::Format("%s=%g", v->GetName(), nextPoint)).getVal(sOpt).first)) { + ::Error("xRooHypoSpace::findlimit", "Problem evaluating %s @ %s=%g", sOpt.Data(), v->GetName(), nextPoint); return lim; } gr.reset(); diff --git a/roofit/xroofit/src/xRooNLLVar.cxx b/roofit/xroofit/src/xRooNLLVar.cxx index c96601236e5df..29ae61277fd03 100644 --- a/roofit/xroofit/src/xRooNLLVar.cxx +++ b/roofit/xroofit/src/xRooNLLVar.cxx @@ -687,7 +687,8 @@ RooArgList xRooNLLVar::xRooFitResult::ranknp(const char *poi, bool up, bool pref auto vv = static_cast(out.at(out.size() - 1)); vv->setVal(v); vv->removeError(); - vv->removeMin();vv->removeMax();//vv->removeRange(); + vv->removeMin(); + vv->removeMax(); // vv->removeRange(); } return out; } @@ -712,7 +713,8 @@ xRooNLLVar::xRooFitResult xRooNLLVar::minimize(const std::shared_ptr(out->constPars()).addClone(RooRealVar(".pgof", "GoF p-value", pgof())); // and just main term - const_cast(out->constPars()).addClone(RooRealVar(".mainterm_pgof", "MainTerm GoF p-value", mainTermPgof())); + const_cast(out->constPars()) + .addClone(RooRealVar(".mainterm_pgof", "MainTerm GoF p-value", mainTermPgof())); } return xRooFitResult(std::make_shared(out, fPdf), std::make_shared(*this)); @@ -743,7 +745,7 @@ double xRooNLLVar::getEntryVal(size_t entry) const *std::unique_ptr(_pdf->getObservables(_data)) = *_data->get(entry); // if (auto s = dynamic_cast(_pdf.get());s) return // -_data->weight()*s->getPdf(s->indexCat().getLabel())->getLogVal(_data->get()); - return -_data->weight() * _pdf->getLogVal(_data->get()); + return (_data->weight() == 0) ? 0 : (-_data->weight() * _pdf->getLogVal(_data->get())); } std::set xRooNLLVar::binnedChannels() const @@ -1593,7 +1595,15 @@ void xRooNLLVar::xRooHypoPoint::Print(Option_t *) const if (!std::isnan(v->getVal())) any_alt = true; } - std::cout << " , pllType: " << fPllType << std::endl; + std::cout << " , pllType: "; + switch (fPllType) { + case 0: std::cout << "qmu"; break; + case 1: std::cout << "qmu or qmutilde"; break; // should check for 'physical' to decide if is latter + case 2: std::cout << "q0"; break; + case 4: std::cout << "u0"; break; + default: std::cout << "unknown"; break; + } + std::cout << std::endl; if (fPllType == xRooFit::Asymptotics::Unknown) { std::cout << " obs ts: " << obs_ts << " +/- " << obs_ts_err << std::endl; @@ -1909,8 +1919,8 @@ xRooNLLVar::xValueWithError xRooNLLVar::xRooHypoPoint::pll(bool readOnly) return std::pair(std::numeric_limits::quiet_NaN(), 0); } if (auto _first_poi = dynamic_cast(poi().first()); - _first_poi && _first_poi->getMin("physical") > _first_poi->getMin() && - mu_hat().getVal() < _first_poi->getMin("physical")) { + fPllType != xRooFit::Asymptotics::Uncapped && _first_poi && + _first_poi->getMin("physical") > _first_poi->getMin() && mu_hat().getVal() < _first_poi->getMin("physical")) { // replace _ufit with fit "boundary" conditional fit _ufit = cfit_lbound(readOnly); if (!_ufit) { @@ -2667,6 +2677,18 @@ xRooNLLVar::hypoPoint(const char *poiValues, double alt_value, const xRooFit::As _type = xRooFit::Asymptotics::OneSidedPositive; } else { _type = xRooFit::Asymptotics::Uncapped; + // for uncapped, should check min is not at physical boundary + for (auto b : out.poi()) { + if (auto r = dynamic_cast(b)) { + if (r->hasRange("physical") && r->getMin() >= r->getMin("physical")) { + ::Info("xRooNLLVar::hypoPoint", + "fitting min of %s is at physical limit, but using uncapped test-statistic, so will set to " + "-max = %g", + r->GetName(), -r->getMax()); + r->setMin(-r->getMax()); + } + } + } } } @@ -3055,7 +3077,8 @@ xRooNLLVar::xRooHypoSpace xRooNLLVar::hypoSpace(const char *parName, int nPoints dynamic_cast(p)->setRange("physical", 0, std::numeric_limits::infinity()); Info("xRooNLLVar::hypoSpace", "Setting physical range of %s to [0,inf]", p->GetName()); } else if (auto v = dynamic_cast(p); v->hasRange("physical")) { - v->removeMin("physical"); v->removeMax("physical");//v->removeRange("physical"); + v->removeMin("physical"); + v->removeMax("physical"); // v->removeRange("physical"); Info("xRooNLLVar::hypoSpace", "Removing physical range of %s", p->GetName()); } } @@ -3072,11 +3095,13 @@ xRooNLLVar::xRooHypoSpace xRooNLLVar::hypoSpace(const char *parName, int nPoints if (nPoints > 0) { out.AddPoints(parName, nPoints, low, high); } else { - if (!std::isnan(low) && !std::isnan(high) && !(std::isinf(low) && std::isinf(high))) { - for (auto p : out.poi()) { - dynamic_cast(p)->setRange("scan", low, high); + // if (!std::isnan(low) && !std::isnan(high) && !(std::isinf(low) && std::isinf(high))) { + for (auto p : out.poi()) { + if (auto r = dynamic_cast(p)) { + r->setRange("scan", std::isnan(low) ? r->getMin() : low, std::isnan(high) ? r->getMax() : high); } } + //} } return out; } @@ -3085,11 +3110,13 @@ xRooNLLVar::xRooHypoSpace xRooNLLVar::hypoSpace(const char *parName, int nPoints if (nPoints > 0) hs.AddPoints(parName, nPoints, low, high); else { - if (!std::isnan(low) && !std::isnan(high) && !(std::isinf(low) && std::isinf(high))) { - for (auto p : hs.poi()) { - dynamic_cast(p)->setRange("scan", low, high); + // if (!std::isnan(low) && !std::isnan(high) && !(std::isinf(low) && std::isinf(high))) { + for (auto p : hs.poi()) { + if (auto r = dynamic_cast(p)) { + r->setRange("scan", std::isnan(low) ? r->getMin() : low, std::isnan(high) ? r->getMax() : high); } } + //} } return hs; } @@ -3124,6 +3151,10 @@ xRooNLLVar::hypoSpace(const char *parName, const xRooFit::Asymptotics::PLLType & for (auto poi : s.poi()) { poi->setStringAttribute("altVal", std::isnan(alt_value) ? nullptr : TString::Format("%f", alt_value)); + // default scan range to range of poi + if (auto r = dynamic_cast(poi)) { + r->setRange("scan", r->getMin(), r->getMax()); + } } return s; @@ -3303,7 +3334,7 @@ std::string cling::printValue(const xRooNLLVar::xValueWithError *v) { if (!v) return "xValueWithError: nullptr\n"; - return Form("%f +/- %f", v->first, v->second); + return Form("%g +/- %g", v->first, v->second); } std::string cling::printValue(const std::map *m) { diff --git a/roofit/xroofit/src/xRooNode.cxx b/roofit/xroofit/src/xRooNode.cxx index 047756e6285fb..2fa78d693775a 100644 --- a/roofit/xroofit/src/xRooNode.cxx +++ b/roofit/xroofit/src/xRooNode.cxx @@ -438,7 +438,8 @@ xRooNode::xRooNode(const char *name, const std::shared_ptr &comp, const "this msg)", int(noErrorPars.size()), (*noErrorPars.begin())->GetName(), (noErrorPars.size() > 1) ? ",..." : ""); if (gEnv->GetValue("XRooFit.SkipInitParErrorInference", false)) { - Warning("xRooNode", "Skipping because XRooFit.SkipInitParErrorInference=true. This is expert-only, you should fix your workspaces!"); + Warning("xRooNode", "Skipping because XRooFit.SkipInitParErrorInference=true. This is expert-only, you " + "should fix your workspaces!"); } else { // get the first top-level pdf browse(); @@ -823,16 +824,22 @@ void xRooNode::Browse(TBrowser *b) formu.ReplaceAll(TString::Format("x[%zu]", i), gv->dependents()[i].GetName()); } _name += formu; - } else if(auto pi = v->get()) { + } else if (auto pi = v->get()) { // check if all interpCodes are the same. std::set interpCodes; - for(auto& c : pi->interpolationCodes()) interpCodes.insert(c); - if(interpCodes.size()==1) { _name += TString::Format(" [InterpCode=%d]",*interpCodes.begin()); } - } else if(auto fiv = v->get()) { + for (auto &c : pi->interpolationCodes()) + interpCodes.insert(c); + if (interpCodes.size() == 1) { + _name += TString::Format(" [InterpCode=%d]", *interpCodes.begin()); + } + } else if (auto fiv = v->get()) { // check if all interpCodes are the same. std::set interpCodes; - for(auto& c : fiv->interpolationCodes()) interpCodes.insert(c==4 ? 5 : c); // in definition of FlexibleInterpVar 4 gets replaced with 5 - if(interpCodes.size()==1) { _name += TString::Format(" [InterpCode=%d]",*interpCodes.begin()); } + for (auto &c : fiv->interpolationCodes()) + interpCodes.insert(c == 4 ? 5 : c); // in definition of FlexibleInterpVar 4 gets replaced with 5 + if (interpCodes.size() == 1) { + _name += TString::Format(" [InterpCode=%d]", *interpCodes.begin()); + } } // tool tip defaults to displaying name and title, so temporarily set name to obj name if has one // and set title to the object type @@ -1357,9 +1364,10 @@ const char *xRooNode::GetIconName() const const char *xRooNode::GetNodeType() const { - if(auto rrs = get(); rrs) { + if (auto rrs = get(); rrs) { // if is BinnedLikelihood show that option - if(rrs->getAttribute("BinnedLikelihood")) return "BinnedLikelihood"; + if (rrs->getAttribute("BinnedLikelihood")) + return "BinnedLikelihood"; } if (auto o = get(); o && fParent && (fParent->get() || fParent->get())) { if (o->InheritsFrom("RooStats::HistFactory::FlexibleInterpVar")) @@ -1536,7 +1544,7 @@ xRooNode xRooNode::Remove(const xRooNode &child) arg = p2->components().find(child.GetName()); if (!arg) throw std::runtime_error(TString::Format("Cannot find %s in %s", child.GetName(), fParent->GetName())); - // remove server ... doesn't seem to trigger removal from proxy + // remove server ... doesn't seem to trigger removal from proxy #if ROOT_VERSION_CODE < ROOT_VERSION(6, 27, 00) p2->_compRSet.remove(*arg); #else @@ -1916,11 +1924,11 @@ xRooNode xRooNode::Add(const xRooNode &child, Option_t *opt) return *this; } auto _arg = child.get(); - if(auto _ds = dynamic_cast(p); _arg && _ds) { + if (auto _ds = dynamic_cast(p); _arg && _ds) { // can add var or function of existing obs to dataset as a column _ds->addColumn(*_arg); _arg->setAttribute("obs"); - return xRooNode(*_arg,*this); + return xRooNode(*_arg, *this); } auto _h = child.get(); if (!_h) { @@ -1992,7 +2000,7 @@ xRooNode xRooNode::Add(const xRooNode &child, Option_t *opt) if (auto p = get(); p) { auto cc = child.fComp; - //bool isConverted = (cc != child.convertForAcquisition(*this, sOpt)); + // bool isConverted = (cc != child.convertForAcquisition(*this, sOpt)); child.convertForAcquisition(*this, sOpt); if ((child.get() || (!child.fComp && getObject(child.GetName())))) { auto out = (child.fComp) ? acquire(child.fComp) : getObject(child.GetName()); @@ -2810,16 +2818,22 @@ void xRooNode::Print(Option_t *opt) const formu.ReplaceAll(TString::Format("x[%zu]", i), gv->dependents()[i].GetName()); } _suffix += formu; - } else if(auto pi = get()) { + } else if (auto pi = get()) { // check if all interpCodes are the same. Will include in the NodeType std::set interpCodes; - for(auto& c : pi->interpolationCodes()) interpCodes.insert(c); - if(interpCodes.size()==1) { _suffix += TString::Format(" [InterpCode=%d]",*interpCodes.begin()); } - } else if(auto fiv = get()) { + for (auto &c : pi->interpolationCodes()) + interpCodes.insert(c); + if (interpCodes.size() == 1) { + _suffix += TString::Format(" [InterpCode=%d]", *interpCodes.begin()); + } + } else if (auto fiv = get()) { // check if all interpCodes are the same. std::set interpCodes; - for(auto& c : fiv->interpolationCodes()) interpCodes.insert(c==4 ? 5 : c); // in definition of FlexibleInterpVar 4 gets replaced with 5 - if(interpCodes.size()==1) { _suffix += TString::Format(" [InterpCode=%d]",*interpCodes.begin()); } + for (auto &c : fiv->interpolationCodes()) + interpCodes.insert(c == 4 ? 5 : c); // in definition of FlexibleInterpVar 4 gets replaced with 5 + if (interpCodes.size() == 1) { + _suffix += TString::Format(" [InterpCode=%d]", *interpCodes.begin()); + } } std::cout << get()->ClassName() << "::" << get()->GetName() << _suffix.Data() << std::endl; } @@ -2880,16 +2894,22 @@ void xRooNode::Print(Option_t *opt) const formu.ReplaceAll(TString::Format("x[%zu]", j), gv->dependents()[j].GetName()); } _suffix += formu; - } else if(auto pi = k->get()) { + } else if (auto pi = k->get()) { // check if all interpCodes are the same. Will include in the NodeType std::set interpCodes; - for(auto& c : pi->interpolationCodes()) interpCodes.insert(c); - if(interpCodes.size()==1) { _suffix += TString::Format(" [InterpCode=%d]",*interpCodes.begin()); } - } else if(auto fiv = k->get()) { + for (auto &c : pi->interpolationCodes()) + interpCodes.insert(c); + if (interpCodes.size() == 1) { + _suffix += TString::Format(" [InterpCode=%d]", *interpCodes.begin()); + } + } else if (auto fiv = k->get()) { // check if all interpCodes are the same. std::set interpCodes; - for(auto& c : fiv->interpolationCodes()) interpCodes.insert(c==4 ? 5 : c); // in definition of FlexibleInterpVar 4 gets replaced with 5 - if(interpCodes.size()==1) { _suffix += TString::Format(" [InterpCode=%d]",*interpCodes.begin()); } + for (auto &c : fiv->interpolationCodes()) + interpCodes.insert(c == 4 ? 5 : c); // in definition of FlexibleInterpVar 4 gets replaced with 5 + if (interpCodes.size() == 1) { + _suffix += TString::Format(" [InterpCode=%d]", *interpCodes.begin()); + } } std::cout << k->get()->ClassName() << "::" << k->get()->GetName() << _suffix.Data() << std::endl; } @@ -4234,7 +4254,8 @@ void xRooNode::_fit_(const char *constParValues, const char *options) : gClient->GetRoot(); TString gofResult = ""; if (_nll.fOpts->find("GoF")) { - gofResult = TString::Format("GoF p-value = %g (mainTerm = %g)\n", fr->constPars().getRealValue(".pgof"),fr->constPars().getRealValue(".mainterm_pgof")); + gofResult = TString::Format("GoF p-value = %g (mainTerm = %g)\n", fr->constPars().getRealValue(".pgof"), + fr->constPars().getRealValue(".mainterm_pgof")); } if (fr->status() != 0) { new TGMsgBox(gClient->GetRoot(), w, "Fit Finished with Bad Status Code", @@ -4943,7 +4964,7 @@ bool xRooNode::SetBinError(int bin, double value) TString origName = (f->getStringAttribute("origName")) ? f->getStringAttribute("origName") : GetName(); rrv->setStringAttribute(Form("sumw2_%s", origName.Data()), TString::Format("%f", pow(value, 2))); auto bin_pars = f->dataHist().get(bin - 1); - auto _binContent = f->dataHist().weight(bin-1); + auto _binContent = f->dataHist().weight(bin - 1); if (f->getAttribute("density")) { _binContent *= f->dataHist().binVolume(*bin_pars); } @@ -5299,9 +5320,10 @@ std::shared_ptr xRooNode::convertForAcquisition(xRooNode &acquirer, con fComp = _f; return _f; - } else if (!get() && (sName.BeginsWith("factory:")||sName.Contains("::")) && acquirer.ws()) { + } else if (!get() && (sName.BeginsWith("factory:") || sName.Contains("::")) && acquirer.ws()) { TString s(sName); - if(sName.BeginsWith("factory:")) s = TString(s(8, s.Length())); + if (sName.BeginsWith("factory:")) + s = TString(s(8, s.Length())); fComp.reset(acquirer.ws()->factory(s), [](TObject *) {}); if (fComp) { const_cast(this)->TNamed::SetName(fComp->GetName()); @@ -6316,14 +6338,11 @@ xRooNode xRooNode::vars() const } } } else if (auto w = get(); w) { - for (auto a : w->allVars()) { - out.emplace_back(std::make_shared(*a, *this)); - out.get()->add(*a); - } - // add all cats as well - for (auto a : w->allCats()) { - out.emplace_back(std::make_shared(*a, *this)); - out.get()->add(*a); + for (auto a : w->components()) { + if (a->InheritsFrom(RooRealVar::Class()) || a->InheritsFrom(RooCategory::Class()) || + a->InheritsFrom(RooConstVar::Class())) { + out.emplace_back(std::make_shared(*a, *this)); + } } } return out; @@ -6930,6 +6949,33 @@ xRooNode xRooNode::datasets() const return out; } +xRooNode xRooNode::parents() const +{ + xRooNode out(".parents", nullptr, *this); + if (auto a = get()) { + for (auto c : a->clients()) { + out.push_back(std::make_shared(*c, *this)); + } + } + return out; +} + +xRooNode xRooNode::args() const +{ + if (auto w = get()) { + xRooNode out(".args", w->components(), *this); + out.browse(); // populate + return out; + } else if (auto a = get()) { + xRooNode out(".args", std::make_shared(), *this); + out.get()->setName((GetPath() + ".args").c_str()); + a->treeNodeServerList(out.get()); + out.browse(); // populate + return out; + } + return nullptr; +} + std::shared_ptr xRooNode::getBrowsable(const char *name) const { for (auto b : fBrowsables) { @@ -7902,7 +7948,10 @@ xRooNode xRooNode::reduced(const std::string &_range, bool invert) const _cat.setLabel(cName); bool matchAny = false; for (auto &p : patterns) { - if (cName.Contains(TRegexp(p, true))) { + TString pNoCatName(p); + if (pNoCatName.Contains('=')) + pNoCatName = pNoCatName(pNoCatName.Index('=') + 1, pNoCatName.Length()); + if (cName.Contains(TRegexp(p, true)) || cName.Contains(TRegexp(pNoCatName, true))) { matchAny = true; break; } @@ -8006,6 +8055,23 @@ xRooNode xRooNode::reduced(const std::string &_range, bool invert) const return get() ? xRooNode(std::make_shared(), fParent) : *this; } +xRooNode xRooNode::reduced(const std::function selector) const +{ + if (empty()) { + const_cast(*this).browse(); + } + // build a list of children to keep + std::string childNames; + for (auto &c : *this) { + if (selector(*c)) { + if (!childNames.empty()) + childNames += ","; + childNames += c->GetName(); + } + } + return reduced(childNames); // calls main method above ... this will ensure we construct a reduced version of ourself +} + // xRooNode xRooNode::generate(bool expected) const { // // auto fr = fitResult(); @@ -8256,7 +8322,7 @@ class PdfWrapper : public RooAbsPdf { } fExpectedEventsMode = expEvMode; } - ~PdfWrapper() override{}; + ~PdfWrapper() override {}; PdfWrapper(const PdfWrapper &other, const char *name = nullptr) : RooAbsPdf(other, name), fFunc("func", this, other.fFunc), @@ -8621,7 +8687,6 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS TObject *vv = rar; - TH1 *h = nullptr; if (!v) { if (binStart != -1 || binEnd != -1) { // allow v to stay nullptr if doing integral (binStart=binEnd=-1) @@ -8872,13 +8937,17 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS for (auto o : _obs) { if (auto rr = o->get(); rr && rr->hasRange("coordRange")) { - rr->removeMin();rr->removeMax();//rr->removeRange("coordRange"); // doesn't actually remove, just sets to -inf->+inf + rr->removeMin(); + rr->removeMax(); // rr->removeRange("coordRange"); // doesn't actually remove, just sets to + // -inf->+inf rr->setStringAttribute("coordRange", nullptr); // removes the attribute } } // probably should also remove any range on the x-axis variable too, if there is one if (auto rr = dynamic_cast(v); rr && rr->hasRange("coordRange")) { - rr->removeMin();rr->removeMax();//rr->removeRange("coordRange"); // doesn't actually remove, just sets to -inf->+inf + rr->removeMin(); + rr->removeMax(); // rr->removeRange("coordRange"); // doesn't actually remove, just sets to + // -inf->+inf rr->setStringAttribute("coordRange", nullptr); // removes the attribute } coords(); // loads current coordinates and populates coordRange, if any @@ -9294,7 +9363,7 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS bool titleMatchName = true; std::map histGroups; std::vector hhs; - std::set> ordered_hhs; + std::set> ordered_hhs; std::set histsWithBadTitles; // these histograms will have their titles autoFormatted // support for CMS model case where has single component containing many coeffs @@ -9323,7 +9392,7 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS // seems I have to remake the function each time, as haven't figured out what cache needs clearing? zero.setAttribute(Form("ORIGNAME:%s", c->GetName())); // used in redirectServers to say what this replaces - forig->redirectServers(RooArgSet(zero), false, true); // each time will replace one additional coef + forig->redirectServers(RooArgSet(zero), false, true); // each time will replace one additional coef std::unique_ptr f(dynamic_cast(forig->Clone("tmpCopy"))); // zero.setAttribute(Form("ORIGNAME:%s",c->GetName()),false); (commented out so that on next iteration @@ -9336,8 +9405,8 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS hh->SetTitle(c->GetName()); // ensure all hists has titles // special case for CMS ... if find "_proc_" in the title, take whatever is after that auto idx = TString(hh->GetTitle()).Index("_proc_"); - if(idx>=0) { - hh->SetTitle(TString(TString(hh->GetTitle())(idx+6,strlen(hh->GetTitle())))); + if (idx >= 0) { + hh->SetTitle(TString(TString(hh->GetTitle())(idx + 6, strlen(hh->GetTitle())))); } histsWithBadTitles.insert(hh); } else if (strcmp(hh->GetName(), hh->GetTitle()) == 0) { @@ -9350,7 +9419,7 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS hh->Scale(-1.); // remove the errors ... the above lines will have introduced errors hh->TH1::Reset("ICE"); // calling the base class method explicitly will only clear errors - ordered_hhs.insert(std::pair(ordered_hhs.size(),hh)); + ordered_hhs.insert(std::pair(ordered_hhs.size(), hh)); prevHist = nextHist; } } else if (get()) { @@ -9379,7 +9448,7 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS titleMatchName &= (TString(samp->GetName()) == hh->GetTitle() || TString(hh->GetTitle()).BeginsWith(TString(samp->GetName()) + "_")); hh->SetBinContent(hh->GetXaxis()->FindFixBin(chanName), samp->GetContent()); - ordered_hhs.insert(std::pair(ordered_hhs.size(),hh)); + ordered_hhs.insert(std::pair(ordered_hhs.size(), hh)); } } } else { @@ -9390,7 +9459,11 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS hh->SetName(samp->GetName()); if (sf) hh->Scale(sf->getVal()); - ordered_hhs.insert(std::pair((samp->get() && samp->get()->getStringAttribute("StackOrder")) ? TString(samp->get()->getStringAttribute("StackOrder")).Atoi() : ordered_hhs.size(),hh)); + ordered_hhs.insert( + std::pair((samp->get() && samp->get()->getStringAttribute("StackOrder")) + ? TString(samp->get()->getStringAttribute("StackOrder")).Atoi() + : ordered_hhs.size(), + hh)); if (strlen(hh->GetTitle()) == 0) { hh->SetTitle(samp->GetName()); // ensure all hists has titles histsWithBadTitles.insert(hh); @@ -9403,7 +9476,7 @@ TH1 *xRooNode::BuildHistogram(RooAbsLValue *v, bool empty, bool errors, int binS } // pull histograms in their order - for(auto& [_,hh] : ordered_hhs) { + for (auto &[_, hh] : ordered_hhs) { hhs.push_back(hh); } @@ -10917,7 +10990,6 @@ void xRooNode::Draw(Option_t *opt) histCopy->SetBit(kCanDelete); auto _axis = (doHorizontal ? histCopy->GetYaxis() : histCopy->GetXaxis()); - graph->GetHistogram()->GetXaxis()->Set(std::max(graph->GetN(), 1), -0.5, std::max(graph->GetN(), 1) - 0.5); for (int ii = 1; ii <= _axis->GetNbins(); ii++) { graph->GetHistogram()->GetXaxis()->SetBinLabel(ii, _axis->GetBinLabel(ii)); @@ -11649,7 +11721,7 @@ void xRooNode::Draw(Option_t *opt) if (!hasSame) clearPad(); - if(rar->getAttribute("Logy")) { + if (rar->getAttribute("Logy")) { gPad->SetLogy(1); } From 4a90a3b2fcbdeebc3fa5e1e5acd42ddeda487a27 Mon Sep 17 00:00:00 2001 From: "will.buttinger" Date: Tue, 12 May 2026 14:15:46 +0100 Subject: [PATCH 2/5] add python library --- roofit/xroofit/CMakeLists.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/roofit/xroofit/CMakeLists.txt b/roofit/xroofit/CMakeLists.txt index c7c0a2638ffd5..8201990b3e051 100644 --- a/roofit/xroofit/CMakeLists.txt +++ b/roofit/xroofit/CMakeLists.txt @@ -17,6 +17,8 @@ ROOT_STANDARD_LIBRARY_PACKAGE(RooFitXRooFit src/xRooNode_interactive.cxx DICTIONARY_OPTIONS "-writeEmptyRootPCM" + LIBRARIES + Python3::Python DEPENDENCIES HistFactory RooFit From 551c7d9c700a94e44c93344cf652e26746a7e303 Mon Sep 17 00:00:00 2001 From: "will.buttinger" Date: Wed, 13 May 2026 15:41:44 +0100 Subject: [PATCH 3/5] ensure printValue is declared outside of namespace --- roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h | 15 +++++++++++---- roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h | 13 ++++++++++--- roofit/xroofit/src/xRooNLLVar.cxx | 10 +++++----- roofit/xroofit/src/xRooNode.cxx | 8 ++++---- 4 files changed, 30 insertions(+), 16 deletions(-) diff --git a/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h b/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h index bd348cef9ff44..296fa51e9ff91 100644 --- a/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h +++ b/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h @@ -511,11 +511,18 @@ class xRooNLLVar : public std::shared_ptr { bool kReuseNLL = true; }; +END_XROOFIT_NAMESPACE + +#ifndef XROOFIT_NAMESPACE_NAME +#ifdef XROOFIT_NAMESPACE +#define XROOFIT_NAMESPACE_NAME XROOFIT_NAMESPACE +#else +#define XROOFIT_NAMESPACE_NAME +#endif +#endif namespace cling { -std::string printValue(const xRooNLLVar::xValueWithError *val); -std::string printValue(const std::map *m); +std::string printValue(const XROOFIT_NAMESPACE_NAME::xRooNLLVar::xValueWithError *val); +std::string printValue(const XROOFIT_NAMESPACE_NAME::std::map *m); } // namespace cling -END_XROOFIT_NAMESPACE - #endif // include guard diff --git a/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h b/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h index 6e00395de831d..ee7c213968ca1 100644 --- a/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h +++ b/roofit/xroofit/inc/RooFit/xRooFit/xRooNode.h @@ -557,10 +557,17 @@ class xRooNode : public TNamed, public std::vector> { ClassDefOverride(xRooNode, 0) }; +END_XROOFIT_NAMESPACE + +#ifndef XROOFIT_NAMESPACE_NAME +#ifdef XROOFIT_NAMESPACE +#define XROOFIT_NAMESPACE_NAME XROOFIT_NAMESPACE +#else +#define XROOFIT_NAMESPACE_NAME +#endif +#endif namespace cling { -std::string printValue(const xRooNode *val); +std::string printValue(const XROOFIT_NAMESPACE_NAME::xRooNode *val); } -END_XROOFIT_NAMESPACE - #endif // include guard diff --git a/roofit/xroofit/src/xRooNLLVar.cxx b/roofit/xroofit/src/xRooNLLVar.cxx index 29ae61277fd03..d67e44022a165 100644 --- a/roofit/xroofit/src/xRooNLLVar.cxx +++ b/roofit/xroofit/src/xRooNLLVar.cxx @@ -3330,13 +3330,15 @@ RooStats::HypoTestResult xRooNLLVar::xRooHypoPoint::result() return out; } -std::string cling::printValue(const xRooNLLVar::xValueWithError *v) +END_XROOFIT_NAMESPACE + +std::string cling::printValue(const XROOFIT_NAMESPACE_NAME::xRooNLLVar::xValueWithError *v) { if (!v) return "xValueWithError: nullptr\n"; return Form("%g +/- %g", v->first, v->second); } -std::string cling::printValue(const std::map *m) +std::string cling::printValue(const XROOFIT_NAMESPACE_NAME::std::map *m) { if (!m) return "nullptr\n"; @@ -3346,6 +3348,4 @@ std::string cling::printValue(const std::map Date: Wed, 13 May 2026 23:52:21 +0100 Subject: [PATCH 4/5] fix namespace issues --- .../xroofit/inc/RooFit/xRooFit/xRooNLLVar.h | 2 +- roofit/xroofit/src/xRooFit.cxx | 6 ++-- roofit/xroofit/src/xRooNLLVar.cxx | 34 +++++++++++-------- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h b/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h index 296fa51e9ff91..949354e131bbc 100644 --- a/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h +++ b/roofit/xroofit/inc/RooFit/xRooFit/xRooNLLVar.h @@ -522,7 +522,7 @@ END_XROOFIT_NAMESPACE #endif namespace cling { std::string printValue(const XROOFIT_NAMESPACE_NAME::xRooNLLVar::xValueWithError *val); -std::string printValue(const XROOFIT_NAMESPACE_NAME::std::map *m); +std::string printValue(const std::map *m); } // namespace cling #endif // include guard diff --git a/roofit/xroofit/src/xRooFit.cxx b/roofit/xroofit/src/xRooFit.cxx index 440eca74bbc13..f4e9e6a510c8f 100644 --- a/roofit/xroofit/src/xRooFit.cxx +++ b/roofit/xroofit/src/xRooFit.cxx @@ -975,7 +975,7 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, if (!out) { int strategy = fitConfig.MinimizerOptions().Strategy(); // Note: AsymptoticCalculator enforces not less than 1 on tolerance - should we do so too? - if (_progress) { + if (_progress && printLevel>=-1) { _nll = new ProgressMonitor(*_nll, _progress); ProgressMonitor::fInterrupt = false; } @@ -1317,7 +1317,7 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, } } - if (miniStrat < _minimizer.fitter()->Config().MinimizerOptions().Strategy() && hesse && + if (printLevel>=-1 && miniStrat < _minimizer.fitter()->Config().MinimizerOptions().Strategy() && hesse && out->edm() > _minimizer.fitter()->Config().MinimizerOptions().Tolerance() * 1e-3 && out->status() != 3) { // hesse may have updated edm by using a better strategy than used in the minimization // so print a warning about this @@ -1434,7 +1434,7 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, fitConfig.MinimizerOptions().SetMinimizerType(actualFirstMinimizer); } - if (_progress) { + if (_progress && printLevel>=-1) { delete _nll; } } diff --git a/roofit/xroofit/src/xRooNLLVar.cxx b/roofit/xroofit/src/xRooNLLVar.cxx index d67e44022a165..1067077da4c75 100644 --- a/roofit/xroofit/src/xRooNLLVar.cxx +++ b/roofit/xroofit/src/xRooNLLVar.cxx @@ -1919,8 +1919,8 @@ xRooNLLVar::xValueWithError xRooNLLVar::xRooHypoPoint::pll(bool readOnly) return std::pair(std::numeric_limits::quiet_NaN(), 0); } if (auto _first_poi = dynamic_cast(poi().first()); - fPllType != xRooFit::Asymptotics::Uncapped && _first_poi && - _first_poi->getMin("physical") > _first_poi->getMin() && mu_hat().getVal() < _first_poi->getMin("physical")) { + _first_poi && _first_poi->getMin("physical") > _first_poi->getMin() && + mu_hat().getVal() < _first_poi->getMin("physical")) { // replace _ufit with fit "boundary" conditional fit _ufit = cfit_lbound(readOnly); if (!_ufit) { @@ -2645,14 +2645,7 @@ xRooNLLVar::hypoPoint(const char *poiValues, double alt_value, const xRooFit::As if (poiNames == "") { throw std::runtime_error("No poi"); } - if (!std::isnan(alt_value)) { - std::unique_ptr thePoi(fFuncVars->selectByName(poiNames)); - for (auto b : *thePoi) { - if (!static_cast(b)->hasRange("physical")) { - static_cast(b)->setRange("physical", 0, std::numeric_limits::infinity()); - } - } - } + auto _snap = std::unique_ptr(fFuncVars->selectByAttrib("Constant", true))->snapshot(); _snap->setAttribAll("poi", false); std::unique_ptr _poi(_snap->selectByName(poiNames)); @@ -2681,10 +2674,11 @@ xRooNLLVar::hypoPoint(const char *poiValues, double alt_value, const xRooFit::As for (auto b : out.poi()) { if (auto r = dynamic_cast(b)) { if (r->hasRange("physical") && r->getMin() >= r->getMin("physical")) { - ::Info("xRooNLLVar::hypoPoint", - "fitting min of %s is at physical limit, but using uncapped test-statistic, so will set to " - "-max = %g", - r->GetName(), -r->getMax()); + ::Info( + "xRooNLLVar::hypoPoint", + "fitting min of %s is >= physical limit (%g), but using uncapped test-statistic, so will set to " + "-max = %g", + r->GetName(), r->getMin("physical"), -r->getMax()); r->setMin(-r->getMax()); } } @@ -2694,6 +2688,16 @@ xRooNLLVar::hypoPoint(const char *poiValues, double alt_value, const xRooFit::As out.fPllType = _type; + // if doing onesidedpositive with an alt value, will assume we need a physical boundary + if (!std::isnan(alt_value) && out.fPllType == xRooFit::Asymptotics::OneSidedPositive) { + std::unique_ptr thePoi(fFuncVars->selectByName(poiNames)); + for (auto b : *thePoi) { + if (!static_cast(b)->hasRange("physical")) { + static_cast(b)->setRange("physical", 0, std::numeric_limits::infinity()); + } + } + } + return out; } @@ -3338,7 +3342,7 @@ std::string cling::printValue(const XROOFIT_NAMESPACE_NAME::xRooNLLVar::xValueWi return "xValueWithError: nullptr\n"; return Form("%g +/- %g", v->first, v->second); } -std::string cling::printValue(const XROOFIT_NAMESPACE_NAME::std::map *m) +std::string cling::printValue(const std::map *m) { if (!m) return "nullptr\n"; From 6b61295e41b8e5215d129f96f4a9c2e103789c61 Mon Sep 17 00:00:00 2001 From: "will.buttinger" Date: Thu, 14 May 2026 16:23:19 +0100 Subject: [PATCH 5/5] more updates --- roofit/xroofit/src/xRooFit.cxx | 27 +++++++++--------- roofit/xroofit/src/xRooHypoSpace.cxx | 6 ++-- roofit/xroofit/src/xRooNLLVar.cxx | 42 ++++++++++++++++++++-------- 3 files changed, 48 insertions(+), 27 deletions(-) diff --git a/roofit/xroofit/src/xRooFit.cxx b/roofit/xroofit/src/xRooFit.cxx index f4e9e6a510c8f..867fb5df4d126 100644 --- a/roofit/xroofit/src/xRooFit.cxx +++ b/roofit/xroofit/src/xRooFit.cxx @@ -548,9 +548,9 @@ void printCerr(const char *msg) { if (Py_IsInitialized()) { PySys_WriteStderr("%s\n", msg); - if (PyObject *sys_stdout = PySys_GetObject("stderr"); sys_stdout != nullptr) { - Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); - } + // if (PyObject *sys_stdout = PySys_GetObject("stderr"); sys_stdout != nullptr) { + // Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); + // } } else { std::cerr << msg << std::endl; } @@ -559,9 +559,9 @@ void printCout(const char *msg) { if (Py_IsInitialized()) { PySys_WriteStdout("%s\n", msg); - if (PyObject *sys_stdout = PySys_GetObject("stdout"); sys_stdout != nullptr) { - Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); - } + // if (PyObject *sys_stdout = PySys_GetObject("stdout"); sys_stdout != nullptr) { + // Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); + // } } else { std::cout << msg << std::endl; } @@ -975,7 +975,7 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, if (!out) { int strategy = fitConfig.MinimizerOptions().Strategy(); // Note: AsymptoticCalculator enforces not less than 1 on tolerance - should we do so too? - if (_progress && printLevel>=-1) { + if (_progress && printLevel >= -2) { _nll = new ProgressMonitor(*_nll, _progress); ProgressMonitor::fInterrupt = false; } @@ -1271,10 +1271,6 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, .Data()); } - if (sIdx >= m_hessestrategy.Length() - 1) { - break; // run out of strategies to try, stop - } - if (_status == 0 && _minimizer.fitter()->GetMinimizer()->CovMatrixStatus() == 3) { // covariance is valid! break; @@ -1282,6 +1278,11 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, // set the statusHistory to the cov status, since that's more informative statusHistory.back().second = _minimizer.fitter()->GetMinimizer()->CovMatrixStatus(); } + + if (sIdx >= m_hessestrategy.Length() - 1) { + break; // run out of strategies to try, stop + } + sIdx++; } // end of hesse attempt loop } @@ -1317,7 +1318,7 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, } } - if (printLevel>=-1 && miniStrat < _minimizer.fitter()->Config().MinimizerOptions().Strategy() && hesse && + if (printLevel >= -2 && miniStrat < _minimizer.fitter()->Config().MinimizerOptions().Strategy() && hesse && out->edm() > _minimizer.fitter()->Config().MinimizerOptions().Tolerance() * 1e-3 && out->status() != 3) { // hesse may have updated edm by using a better strategy than used in the minimization // so print a warning about this @@ -1434,7 +1435,7 @@ std::shared_ptr xRooFit::minimize(RooAbsReal &nll, fitConfig.MinimizerOptions().SetMinimizerType(actualFirstMinimizer); } - if (_progress && printLevel>=-1) { + if (_progress && printLevel >= -2) { delete _nll; } } diff --git a/roofit/xroofit/src/xRooHypoSpace.cxx b/roofit/xroofit/src/xRooHypoSpace.cxx index 0f13ac3d6e896..bc00d6906a586 100644 --- a/roofit/xroofit/src/xRooHypoSpace.cxx +++ b/roofit/xroofit/src/xRooHypoSpace.cxx @@ -648,9 +648,9 @@ xRooNLLVar::xRooHypoPoint &xRooNLLVar::xRooHypoSpace::AddPoint(const char *coord if (Py_IsInitialized()) { auto s = TString::Format("Info in : Added new point @ %s", coordString.c_str()); PySys_WriteStdout("%s\n", s.Data()); - if (PyObject *sys_stdout = PySys_GetObject("stdout"); sys_stdout != nullptr) { - Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); - } + // if (PyObject *sys_stdout = PySys_GetObject("stdout"); sys_stdout != nullptr) { + // Py_XDECREF(PyObject_CallMethod(sys_stdout, "flush", nullptr)); + // } } else { ::Info("xRooHypoSpace::AddPoint", "Added new point @ %s", coordString.c_str()); } diff --git a/roofit/xroofit/src/xRooNLLVar.cxx b/roofit/xroofit/src/xRooNLLVar.cxx index 1067077da4c75..4cfdde7b38d93 100644 --- a/roofit/xroofit/src/xRooNLLVar.cxx +++ b/roofit/xroofit/src/xRooNLLVar.cxx @@ -1597,7 +1597,7 @@ void xRooNLLVar::xRooHypoPoint::Print(Option_t *) const } std::cout << " , pllType: "; switch (fPllType) { - case 0: std::cout << "qmu"; break; + case 0: std::cout << "tmu"; break; case 1: std::cout << "qmu or qmutilde"; break; // should check for 'physical' to decide if is latter case 2: std::cout << "q0"; break; case 4: std::cout << "u0"; break; @@ -1775,6 +1775,14 @@ std::shared_ptr xRooNLLVar::xRooHypoPoint::asimov(boo // dynamic_cast(p)->removeRange("physical"); -- can't use this as will modify shared property if (auto v = dynamic_cast(p)) { v->deleteSharedProperties(); // effectively removes all custom ranges + if (v->getVal() == 0) { + // for discovery tests, we generate asimov at mu!=0 and then evaluate the two sided + // at some value of mu. Normally we would use mu=0 but if we have a bin + // with only signal contribution (no bkg) will get asimov data in that bin + // and no prediction ... the cfit(mu=0) will never succeed on this + // so lets move to half the alt value instead (the value used to generate) + v->setVal(theFit->constPars().getRealValue(v->GetName()) * 0.5); + } } } @@ -1799,15 +1807,19 @@ xRooNLLVar::xValueWithError xRooNLLVar::xRooHypoPoint::pNull_asymp(double nSigma auto first_poi = dynamic_cast(poi().first()); if (!first_poi) return std::pair(std::numeric_limits::quiet_NaN(), 0); - auto _sigma_mu = sigma_mu(); + double lowBound = first_poi->getMin("physical"); + double hiBound = first_poi->getMax("physical"); + // don't need to calculate sigma_mu if physical boundaries at infinity, PValue doesn't depend on it + auto _sigma_mu = + (lowBound == -std::numeric_limits::infinity() && hiBound == std::numeric_limits::infinity()) + ? std::pair(0, 0) + : sigma_mu(); double nom = xRooFit::Asymptotics::PValue(fPllType, ts_asymp(nSigma).first, fNullVal(), fNullVal(), _sigma_mu.first, - first_poi->getMin("physical"), first_poi->getMax("physical")); - double up = - xRooFit::Asymptotics::PValue(fPllType, ts_asymp(nSigma).first + ts_asymp(nSigma).second, fNullVal(), fNullVal(), - _sigma_mu.first, first_poi->getMin("physical"), first_poi->getMax("physical")); - double down = - xRooFit::Asymptotics::PValue(fPllType, ts_asymp(nSigma).first - ts_asymp(nSigma).second, fNullVal(), fNullVal(), - _sigma_mu.first, first_poi->getMin("physical"), first_poi->getMax("physical")); + lowBound, hiBound); + double up = xRooFit::Asymptotics::PValue(fPllType, ts_asymp(nSigma).first + ts_asymp(nSigma).second, fNullVal(), + fNullVal(), _sigma_mu.first, lowBound, hiBound); + double down = xRooFit::Asymptotics::PValue(fPllType, ts_asymp(nSigma).first - ts_asymp(nSigma).second, fNullVal(), + fNullVal(), _sigma_mu.first, lowBound, hiBound); return std::pair(nom, std::max(std::abs(up - nom), std::abs(down - nom))); } @@ -2248,8 +2260,8 @@ xRooNLLVar::xValueWithError xRooNLLVar::xRooHypoPoint::sigma_mu(bool readOnly) } auto out = asi->pll(readOnly); - return std::pair(std::abs(fNullVal() - fAltVal()) / sqrt(out.first), - out.second * 0.5 * std::abs(fNullVal() - fAltVal()) / + return std::pair(std::abs(asi->fNullVal() - fAltVal()) / sqrt(out.first), + out.second * 0.5 * std::abs(asi->fNullVal() - fAltVal()) / (out.first * sqrt(out.first))); } @@ -2615,6 +2627,10 @@ xRooNLLVar::hypoPoint(const char *poiValues, double alt_value, const xRooFit::As AutoRestorer snap(*fFuncVars); out.nllVar = std::make_shared(*this); + // clear the underlying RooAbsReal, so that we don't accidentally alter it (e.g. setAttribute readOnly) + // and therefore alter the xRooNLLVar object we are creating this hypoPoint from + // basically ensure the hypoPoint has an independent version of the function + out.nllVar->reset(); out.fData = getData(); TStringToken pattern(poiValues, ","); @@ -3151,6 +3167,10 @@ xRooNLLVar::hypoSpace(const char *parName, const xRooFit::Asymptotics::PLLType & throw std::runtime_error("You must specify at least one POI for the hypoSpace"); }*/ s.fNlls[s.fPdfs.begin()->second] = std::make_shared(*this); + // clear the underlying RooAbsReal, so that we don't accidentally alter it (e.g. setAttribute readOnly) + // and therefore alter the xRooNLLVar object we are creating this hypoPoint from + // basically ensure the hypoPoint has an independent version of the function + s.fNlls[s.fPdfs.begin()->second]->reset(); s.fTestStatType = pllType; for (auto poi : s.poi()) {