From 65ff8be81d96d389a4bd0b983d8acc224b3bc1b6 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 14 Aug 2025 17:11:03 +0200 Subject: [PATCH 1/2] Don't hide categories when creating the RooMinimizer Indeed, the RooMinimizer can't deal with floating category variables. But instead of hiding the categories when creating the RooMinimizer, one can also set them to constant and the minimizer will not complain too. This solution is less hacky than hooking into `getParameters()` of the CachingSimNLL. This was originally introduced by @gpetrucc in https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/pull/574 Quoting from that PR: ``` For the dirty flag propagation to the NLL when the categories are changed, and option to avoid reporting the categories in getParameters() to avoid complaints from RooMinimizer ``` So indeed, hiding the categories was supposed to address these warnings, which are also gone when the categories are just set to constant. --- interface/CachingNLL.h | 2 -- src/CachingNLL.cc | 9 ++++----- src/CascadeMinimizer.cc | 22 +++++++++++++++++----- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/interface/CachingNLL.h b/interface/CachingNLL.h index 8145881a7a7..e6ac39904ac 100644 --- a/interface/CachingNLL.h +++ b/interface/CachingNLL.h @@ -178,7 +178,6 @@ class CachingSimNLL : public RooAbsReal { static void forceUnoptimizedConstraints() { optimizeContraints_ = false; } void setChannelMasks(RooArgList const& args); void setAnalyticBarlowBeeston(bool flag); - void setHideRooCategories(bool flag) { hideRooCategories_ = flag; } void setHideConstants(bool flag) { hideConstants_ = flag; } void setMaskConstraints(bool flag) ; void setMaskNonDiscreteChannels(bool mask) ; @@ -191,7 +190,6 @@ class CachingSimNLL : public RooAbsReal { const RooAbsData *dataOriginal_; const RooArgSet *nuis_; RooSetProxy params_, catParams_; - bool hideRooCategories_ = false; bool hideConstants_ = false; RooArgSet piecesForCloning_; std::unique_ptr factorizedPdf_; diff --git a/src/CachingNLL.cc b/src/CachingNLL.cc index 8b2578392e8..714b23216db 100644 --- a/src/CachingNLL.cc +++ b/src/CachingNLL.cc @@ -847,7 +847,6 @@ cacheutils::CachingSimNLL::CachingSimNLL(const CachingSimNLL &other, const char nuis_(other.nuis_), params_("params","parameters",this), catParams_("catParams","Category parameters",this), - hideRooCategories_(other.hideRooCategories_), hideConstants_(other.hideConstants_), internalMasks_(other.internalMasks_), maskConstraints_(other.maskConstraints_), @@ -1253,10 +1252,10 @@ cacheutils::CachingSimNLL::getParameters(const RooArgSet* depList, Bool_t stripD RooArgSet *ret; if (internalMasks_.empty()) { ret = new RooArgSet(params_); - if (!hideRooCategories_) ret->add(catParams_); + ret->add(catParams_); } else { ret = new RooArgSet(activeParameters_); - if (!hideRooCategories_) ret->add(activeCatParameters_); + ret->add(activeCatParameters_); } if (hideConstants_) RooStats::RemoveConstantParameters(ret); return ret; @@ -1268,10 +1267,10 @@ bool cacheutils::CachingSimNLL::getParameters(const RooArgSet* depList, { if (internalMasks_.empty()) { outputSet.add(params_); - if (!hideRooCategories_) outputSet.add(catParams_); + outputSet.add(catParams_); } else { outputSet.add(activeParameters_); - if (!hideRooCategories_) outputSet.add(activeCatParameters_); + outputSet.add(activeCatParameters_); } if (hideConstants_) RooStats::RemoveConstantParameters(&outputSet); return true; diff --git a/src/CascadeMinimizer.cc b/src/CascadeMinimizer.cc index 76f1992cd26..bea981a938f 100644 --- a/src/CascadeMinimizer.cc +++ b/src/CascadeMinimizer.cc @@ -53,11 +53,23 @@ CascadeMinimizer::CascadeMinimizer(RooAbsReal &nll, Mode mode, RooRealVar *poi) } void CascadeMinimizer::remakeMinimizer() { - cacheutils::CachingSimNLL *simnll = dynamic_cast(&nll_); - if (simnll) simnll->setHideRooCategories(true); - minimizer_.reset(); // avoid two copies in memory - minimizer_.reset(new RooMinimizer(nll_)); - if (simnll) simnll->setHideRooCategories(false); + std::unique_ptr nllParams(nll_.getParameters((const RooArgSet*)0)); + RooArgSet changedSet; + + // The RooMinimizer can't deal with floating categories, so we need to set them constant at least while creating the minimizer. + for (RooAbsArg *arg : *nllParams) { + auto cat = dynamic_cast(arg); + if (cat && !cat->isConstant()) { + cat->setConstant(true); + changedSet.add(*cat); + } + } + + minimizer_.reset(); // avoid two copies in memory + minimizer_.reset(new RooMinimizer(nll_)); + + // Change categories back to how they were + utils::setAllConstant(changedSet, false); } bool CascadeMinimizer::freezeDiscParams(const bool freeze) From 0225c818ef6afd023e57fe1869ad74d810ca7bf5 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 14 Aug 2025 17:28:43 +0200 Subject: [PATCH 2/2] Remove optional hiding of constants and constraints Originally added in 2019 by @gpetrucc in https://github.com/cms-analysis/HiggsAnalysis-CombinedLimit/pull/576. I quote that PR: ``` It is possible that already setting maskChannels=2 and SIMNLL_GROUPCONSTRAINTS=N make the gain from hideConstant and maskConstraints redundant in practice but I haven't tested since I developed them in the order they're listed here. ``` I think this is indeed the case. Maybe it would be good to check if removing these options indeed has relevant effect on performance, because reducing the number of these RooFit hacks helps to converge between Combine and upstream RooFit. --- docs/part3/nonstandard.md | 2 -- interface/CachingNLL.h | 4 ---- src/CachingNLL.cc | 15 +-------------- src/CascadeMinimizer.cc | 12 ------------ 4 files changed, 1 insertion(+), 32 deletions(-) diff --git a/docs/part3/nonstandard.md b/docs/part3/nonstandard.md index d54c9dc9eab..dcd94dd7359 100644 --- a/docs/part3/nonstandard.md +++ b/docs/part3/nonstandard.md @@ -568,8 +568,6 @@ As expected, the curve obtained by allowing the `pdf_index` to float (labelled " In general, the performance of Combine can be improved when using the discrete profiling method by including the option `--X-rtd MINIMIZER_freezeDisassociatedParams`. This will stop parameters not associated to the current PDF from floating in the fits. Additionally, you can include the following options: -- `--X-rtd MINIMIZER_multiMin_hideConstants`: hide the constant terms in the likelihood when recreating the minimizer -- `--X-rtd MINIMIZER_multiMin_maskConstraints`: hide the constraint terms during the discrete minimization process - `--X-rtd MINIMIZER_multiMin_maskChannels=` mask the channels that are not needed from the NLL: - ` 1`: keeps unmasked all channels that are participating in the discrete minimization. - ` 2`: keeps unmasked only the channel whose index is being scanned at the moment. diff --git a/interface/CachingNLL.h b/interface/CachingNLL.h index e6ac39904ac..e69ec83b3ff 100644 --- a/interface/CachingNLL.h +++ b/interface/CachingNLL.h @@ -178,8 +178,6 @@ class CachingSimNLL : public RooAbsReal { static void forceUnoptimizedConstraints() { optimizeContraints_ = false; } void setChannelMasks(RooArgList const& args); void setAnalyticBarlowBeeston(bool flag); - void setHideConstants(bool flag) { hideConstants_ = flag; } - void setMaskConstraints(bool flag) ; void setMaskNonDiscreteChannels(bool mask) ; friend class CachingAddNLL; // trap this call, since we don't care about propagating it to the sub-components @@ -190,7 +188,6 @@ class CachingSimNLL : public RooAbsReal { const RooAbsData *dataOriginal_; const RooArgSet *nuis_; RooSetProxy params_, catParams_; - bool hideConstants_ = false; RooArgSet piecesForCloning_; std::unique_ptr factorizedPdf_; std::vector constrainPdfs_; @@ -210,7 +207,6 @@ class CachingSimNLL : public RooAbsReal { std::vector constrainZeroPointsFastPoisson_; std::vector channelMasks_; std::vector internalMasks_; - bool maskConstraints_ = false; RooArgSet activeParameters_, activeCatParameters_; double maskingOffset_ = 0; // offset to ensure that interal or constraint masking doesn't change NLL value double maskingOffsetZero_ = 0; // and associated zero point diff --git a/src/CachingNLL.cc b/src/CachingNLL.cc index 714b23216db..67008ad3d96 100644 --- a/src/CachingNLL.cc +++ b/src/CachingNLL.cc @@ -847,9 +847,7 @@ cacheutils::CachingSimNLL::CachingSimNLL(const CachingSimNLL &other, const char nuis_(other.nuis_), params_("params","parameters",this), catParams_("catParams","Category parameters",this), - hideConstants_(other.hideConstants_), internalMasks_(other.internalMasks_), - maskConstraints_(other.maskConstraints_), maskingOffset_(other.maskingOffset_), maskingOffsetZero_(other.maskingOffsetZero_) { @@ -1048,7 +1046,7 @@ cacheutils::CachingSimNLL::evaluate() const ret += nllval; } } - if (!maskConstraints_ && (!constrainPdfs_.empty() || !constrainPdfsFast_.empty() || !constrainPdfsFastPoisson_.empty() || !constrainPdfGroups_.empty())) { + if (!constrainPdfs_.empty() || !constrainPdfsFast_.empty() || !constrainPdfsFastPoisson_.empty() || !constrainPdfGroups_.empty()) { DefaultAccumulator ret2 = 0; /// ============= GENERIC CONSTRAINTS ========= std::vector::const_iterator itz = constrainZeroPoints_.begin(); @@ -1257,7 +1255,6 @@ cacheutils::CachingSimNLL::getParameters(const RooArgSet* depList, Bool_t stripD ret = new RooArgSet(activeParameters_); ret->add(activeCatParameters_); } - if (hideConstants_) RooStats::RemoveConstantParameters(ret); return ret; } #else @@ -1272,20 +1269,10 @@ bool cacheutils::CachingSimNLL::getParameters(const RooArgSet* depList, outputSet.add(activeParameters_); outputSet.add(activeCatParameters_); } - if (hideConstants_) RooStats::RemoveConstantParameters(&outputSet); return true; } #endif -void cacheutils::CachingSimNLL::setMaskConstraints(bool flag) { - double nllBefore = evaluate(); - maskConstraints_ = flag; - double nllAfter = evaluate(); - maskingOffset_ += (nllBefore - nllAfter); - //printf("CachingSimNLL: setMaskConstraints(%d): nll before %.12g, nll after %.12g (diff %.12g), new maskingOffset %.12g, check = %.12g\n", - // int(flag), nllBefore, nllAfter, (nllBefore-nllAfter), maskingOffset_, evaluate() - nllBefore); -} - void cacheutils::CachingSimNLL::setMaskNonDiscreteChannels(bool mask) { double nllBefore = evaluate(); internalMasks_.clear(); // reset diff --git a/src/CascadeMinimizer.cc b/src/CascadeMinimizer.cc index bea981a938f..f453d5f5a6c 100644 --- a/src/CascadeMinimizer.cc +++ b/src/CascadeMinimizer.cc @@ -496,8 +496,6 @@ bool CascadeMinimizer::minimize(int verbose, bool cascade) bool CascadeMinimizer::multipleMinimize(const RooArgSet &reallyCleanParameters, bool& ret, double& minimumNLL, int verbose, bool cascade,int mode, std::vector >&contributingIndeces){ static bool freezeDisassParams = runtimedef::get(std::string("MINIMIZER_freezeDisassociatedParams")); - static bool hideConstants = freezeDisassParams && runtimedef::get(std::string("MINIMIZER_multiMin_hideConstants")); - static bool maskConstraints = freezeDisassParams && runtimedef::get(std::string("MINIMIZER_multiMin_maskConstraints")); static int maskChannels = freezeDisassParams ? runtimedef::get(std::string("MINIMIZER_multiMin_maskChannels")) : 0; cacheutils::CachingSimNLL *simnll = dynamic_cast(&nll_); @@ -564,11 +562,6 @@ bool CascadeMinimizer::multipleMinimize(const RooArgSet &reallyCleanParameters, if (maskChannels && simnll) { simnll->setMaskNonDiscreteChannels(true); } - if (hideConstants && simnll) { - simnll->setHideConstants(true); - if (maskConstraints) simnll->setMaskConstraints(true); - minimizer_.reset(); // will be recreated when needed by whoever needs it - } std::vector > myCombos; @@ -706,11 +699,6 @@ bool CascadeMinimizer::multipleMinimize(const RooArgSet &reallyCleanParameters, if (maskChannels && simnll) { simnll->setMaskNonDiscreteChannels(false); } - if (hideConstants && simnll) { - simnll->setHideConstants(false); - if (maskConstraints) simnll->setMaskConstraints(false); - minimizer_.reset(); // will be recreated when needed by whoever needs it - } return newDiscreteMinimum;