[RF] Address the RooFormula re-indexing issue during serialization

Author: tekinertekin

roofit/roofitcore/src/RooFormula.cxx

@@ -389,6 +389,54 @@ RooArgList RooFormula::usedVariables() const {
   return useList;
 }
 
+////////////////////////////////////////////////////////////////////////////////
+/// Reindex the formula expression to map only the variables that are actually in use.
+/// Return the processed formula string with the `x[i]` positional indices
+/// remapped to each variable's position in usedVariables() (the pruned list of
+/// actually-used servers) instead of the full original list. This keeps the
+/// persisted pair (formula string, actualDependents()) self-consistent, so a
+/// RooFormulaVar or RooGenericPdf survives a write/read cycle even when unused
+/// parameters were pruned. See https://github.com/root-project/root/issues/21371
+/// \return A new formula string with reindexed variable placeholders.
+std::string RooFormula::reindexedFormulaForUsedVars() const
+{
+   const std::string processedFormula(_tFormula->GetTitle());
+
+   int unUsedCount = 0;
+   std::vector<int> newIndex;
+   newIndex.reserve(_varIsUsed.size());
+   // Map each original index to its position among the used variables;
+   // pruned entries get -1 and are never looked up (they don't appear in the formula).
+   for (std::size_t i = 0; i < _varIsUsed.size(); ++i) {
+      if (!_varIsUsed[i]) {
+         unUsedCount++;
+         newIndex.push_back(-1);
+      } else {
+         newIndex.push_back(static_cast<int>(i) - unUsedCount);
+      }
+   }
+
+   static const std::regex newOrdinalRegex("\\bx\\[([0-9]+)\\]");
+
+   std::string result;
+   std::size_t lastPos = 0;
+   result.reserve(processedFormula.size());
+   // Single pass: rewrite every x[old] to x[newIndex[old]].
+   for (sregex_iterator matchIt = sregex_iterator(processedFormula.begin(), processedFormula.end(), newOrdinalRegex);
+        matchIt != sregex_iterator(); ++matchIt) {
+      std::smatch match = *matchIt;
+
+      result.append(processedFormula, lastPos, match.position() - lastPos);
+      const int oldIdx = std::stoi(match[1].str());
+      result += "x[" + std::to_string(newIndex[oldIdx]) + "]";
+
+      lastPos = match.position() + match.length();
+   }
+   result.append(processedFormula, lastPos, std::string::npos);
+
+   return result;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 /// Change used variables to those with the same name in given list.
 /// \param[in] newDeps New dependents to replace the old ones.

roofit/roofitcore/src/RooFormula.h

@@ -49,6 +49,7 @@ class RooFormula : public TNamed {
    void printMultiline(std::ostream &os, Int_t contents, bool verbose = false, TString indent = "") const;
 
    std::string formulaString() const { return _tFormula ? _tFormula->GetTitle() : ""; }
+   std::string reindexedFormulaForUsedVars() const;
    TFormula* getTFormula() const { return _tFormula.get(); }
 
 private:

roofit/roofitcore/src/RooFormulaVar.cxx

@@ -89,7 +89,7 @@ RooFormulaVar::RooFormulaVar(const char *name, const char *title, const char* in
     _value = traceEval(nullptr);
   } else {
     _formula = new RooFormula(GetName(), _formExpr, dependents, checkVariables);
-    _formExpr = _formula->formulaString().c_str();
+    _formExpr = _formula->reindexedFormulaForUsedVars().c_str();
     _actualVars.add(_formula->actualDependents());
   }
 }
@@ -112,7 +112,7 @@ RooFormulaVar::RooFormulaVar(const char *name, const char *title, const RooArgLi
     _value = traceEval(nullptr);
   } else {
     _formula = new RooFormula(GetName(), _formExpr, dependents, checkVariables);
-    _formExpr = _formula->formulaString().c_str();
+    _formExpr = _formula->reindexedFormulaForUsedVars().c_str();
     _actualVars.add(_formula->actualDependents());
   }
 }
@@ -132,7 +132,7 @@ RooFormulaVar::RooFormulaVar(const RooFormulaVar& other, const char* name) :
    }
   if (other._formula && other._formula->ok()) {
     _formula = new RooFormula(*other._formula);
-    _formExpr = _formula->formulaString().c_str();
+    _formExpr = _formula->reindexedFormulaForUsedVars().c_str();
   }
 }
 
@@ -145,7 +145,7 @@ RooFormula& RooFormulaVar::getFormula() const
   if (!_formula) {
     // After being read from file, the formula object might not exist, yet:
     _formula = new RooFormula(GetName(), _formExpr, _actualVars);
-    const_cast<TString&>(_formExpr) = _formula->formulaString().c_str();
+    const_cast<TString &>(_formExpr) = _formula->reindexedFormulaForUsedVars().c_str();
   }
 
   return *_formula;
@@ -180,7 +180,7 @@ bool RooFormulaVar::redirectServersHook(const RooAbsCollection& newServerList, b
 {
   bool error = getFormula().changeDependents(newServerList,mustReplaceAll,nameChange);
 
-  _formExpr = getFormula().GetTitle();
+  _formExpr = getFormula().reindexedFormulaForUsedVars().c_str();
   return error || RooAbsReal::redirectServersHook(newServerList, mustReplaceAll, nameChange, isRecursive);
 }
 

roofit/roofitcore/src/RooGenericPdf.cxx

@@ -76,7 +76,7 @@ RooGenericPdf::RooGenericPdf(const char *name, const char *title, const RooArgLi
     _value = traceEval(nullptr);
   } else {
     _formula = new RooFormula(GetName(), _formExpr, dependents);
-    _formExpr = _formula->formulaString().c_str();
+    _formExpr = _formula->reindexedFormulaForUsedVars().c_str();
     _actualVars.add(_formula->actualDependents());
   }
 }
@@ -96,7 +96,7 @@ RooGenericPdf::RooGenericPdf(const char *name, const char *title,
     _value = traceEval(nullptr);
   } else {
     _formula = new RooFormula(GetName(), _formExpr, dependents);
-    _formExpr = _formula->formulaString().c_str();
+    _formExpr = _formula->reindexedFormulaForUsedVars().c_str();
     _actualVars.add(_formula->actualDependents());
   }
 }
@@ -124,7 +124,7 @@ RooFormula& RooGenericPdf::formula() const
 {
   if (!_formula) {
     _formula = new RooFormula(GetName(),_formExpr.Data(),_actualVars);
-    const_cast<TString&>(_formExpr) = _formula->formulaString().c_str();
+    const_cast<TString &>(_formExpr) = _formula->reindexedFormulaForUsedVars().c_str();
   }
   return *_formula ;
 }

roofit/roofitcore/test/testGenericPdf.cxx

@@ -1,6 +1,7 @@
 // Tests for the GenericPdf
 // Authors: Stephan Hageboeck, CERN  05/2019
 //          Jonas Rembser, CERN 06/2022
+#include <TFile.h>
 
 #include <RooArgList.h>
 #include <RooBinning.h>
@@ -415,3 +416,28 @@ TEST(GenericPdf, BinnedBoundariesConsistentWithHistPdf)
       iterFormulaVar++;
    }
 }
+
+// Regression test for https://github.com/root-project/root/issues/21371:
+// an unused parameter (b) is pruned, so the persisted @N indices must be
+// remapped or the formula silently mismaps after a write/read cycle.
+TEST(GenericPdf, SerializationWithUnusedParam)
+{
+   RooWorkspace w("w");
+   w.factory("a[2,-10,10]");
+   w.factory("b[99,-10,10]");
+   w.factory("c[3,-10,10]");
+   w.factory("d[4,-10,10]");
+   w.factory("EXPR::pdf('@0*@2+d', a, b, c, d)");
+
+   TString fn = "RooGenericPdfSerialization.root";
+   w.writeToFile(fn);
+   TFile fin(fn);
+   RooWorkspace *w2 = nullptr;
+   fin.GetObject("w", w2);
+   ASSERT_NE(w2, nullptr);
+   auto *pdf = static_cast<RooAbsReal *>(w2->pdf("pdf"));
+
+   // If @2 still maps to c, changing c updates pdf = a*c + d = 2*5 + 4 = 14.
+   static_cast<RooRealVar *>(w2->var("c"))->setVal(5.0);
+   EXPECT_DOUBLE_EQ(pdf->getVal(), 2.0 * 5.0 + 4.0);
+}

roofit/roofitcore/test/testRooFormula.cxx

@@ -2,11 +2,13 @@
 // Authors: Stephan Hageboeck, CERN  2020
 //          Jonas Rembser, CERN 2023
 //          Andrea Germinario, CERN 2025
+#include <TFile.h>
 
 #include "../src/RooFormula.h"
 #include <RooFormulaVar.h>
 #include <RooRealVar.h>
 #include <RooConstVar.h>
+#include <RooWorkspace.h>
 
 #include <ROOT/TestSupport.hxx>
 
@@ -92,6 +94,31 @@ TEST(RooFormula, UndefinedVariables)
    ASSERT_NO_THROW(RooFormulaVar f2("f2", "r + B + y", {r, B, y})) << "Formula with specified y must work.";
 }
 
+// Regression test for https://github.com/root-project/root/issues/21371:
+// an unused parameter (b) is pruned, so the persisted @N indices must be
+// remapped or the formula silently mismaps after a write/read cycle.
+TEST(RooFormula, SerializationWithUnusedParam)
+{
+   RooWorkspace w("w");
+   w.factory("a[2,-10,10]");
+   w.factory("b[99,-10,10]");
+   w.factory("c[3,-10,10]");
+   w.factory("d[4,-10,10]");
+   w.factory("expr::f('@0*@2+d', a, b, c, d)");
+
+   TString fn = "RooFormulaSerialization.root";
+   w.writeToFile(fn);
+   TFile fin(fn);
+   RooWorkspace *w2 = nullptr;
+   fin.GetObject("w", w2);
+   ASSERT_NE(w2, nullptr);
+   auto *f = static_cast<RooAbsReal *>(w2->function("f"));
+
+   // If @2 still maps to c, changing c updates f = a*c + d = 2*5 + 4 = 14.
+   static_cast<RooRealVar *>(w2->var("c"))->setVal(5.0);
+   EXPECT_DOUBLE_EQ(f->getVal(), 2.0 * 5.0 + 4.0);
+}
+
 TEST(RooFormula, RooConstVarSafeSubstitution)
 {
    // Check RooConst are substituted only by index