Remove some more getParameters() hacks

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 <span style="font-variant:small-caps;">Combine</span> 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=<choice>` mask the channels that are not needed from the NLL:
   - `<choice> 1`: keeps unmasked all channels that are participating in the discrete minimization.
   - `<choice> 2`: keeps unmasked only the channel whose index is being scanned at the moment.

interface/CachingNLL.h

@@ -182,9 +182,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) ;
         friend class CachingAddNLL;
         // trap this call, since we don't care about propagating it to the sub-components
@@ -195,8 +192,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<RooSimultaneous>  factorizedPdf_;
         std::vector<RooAbsPdf *>        constrainPdfs_;
@@ -216,7 +211,6 @@ class CachingSimNLL  : public RooAbsReal {
         std::vector<double> constrainZeroPointsFastPoisson_;
         std::vector<RooAbsReal*> channelMasks_;
         std::vector<bool>        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

src/CachingNLL.cc

@@ -838,10 +838,7 @@ 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_),
     maskingOffset_(other.maskingOffset_),
     maskingOffsetZero_(other.maskingOffsetZero_)
 {
@@ -1048,7 +1045,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<double> ret2 = 0;
         /// ============= GENERIC CONSTRAINTS  =========
         for (std::size_t i = 0; i < constrainPdfs_.size(); ++i) {
@@ -1240,12 +1237,11 @@ 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;
 }
 #else
@@ -1255,25 +1251,15 @@ 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;
 }
 #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

src/CascadeMinimizer.cc

@@ -14,6 +14,7 @@
 #include <RooStats/RooStatsUtils.h>
 
 #include <iomanip>
+#include <memory>
 
 boost::program_options::options_description CascadeMinimizer::options_("Cascade Minimizer options");
 std::vector<CascadeMinimizer::Algo> CascadeMinimizer::fallbacks_;
@@ -53,11 +54,23 @@ CascadeMinimizer::CascadeMinimizer(RooAbsReal &nll, Mode mode, RooRealVar *poi)
 }
 
 void CascadeMinimizer::remakeMinimizer() {
-    cacheutils::CachingSimNLL *simnll = dynamic_cast<cacheutils::CachingSimNLL *>(&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<RooArgSet> nllParams(nll_.getParameters((const RooArgSet*)nullptr));
+  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<RooCategory *>(arg);
+    if (cat && !cat->isConstant()) {
+      cat->setConstant(true);
+      changedSet.add(*cat);
+    }
+  }
+
+  minimizer_.reset();  // avoid two copies in memory
+  minimizer_ = std::make_unique<RooMinimizer>(nll_);
+
+  // Change categories back to how they were
+  utils::setAllConstant(changedSet, /*constant=*/false);
 }
 
 bool CascadeMinimizer::freezeDiscParams(const bool freeze)
@@ -502,8 +515,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<std::vector<bool> >&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<cacheutils::CachingSimNLL *>(&nll_);
 
@@ -570,11 +581,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<std::vector<int> > myCombos;
 
@@ -712,11 +718,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;