diff --git a/roofit/roofitcore/src/RooFormula.cxx b/roofit/roofitcore/src/RooFormula.cxx index da76eb8cd5294..6d4cb186404bc 100644 --- a/roofit/roofitcore/src/RooFormula.cxx +++ b/roofit/roofitcore/src/RooFormula.cxx @@ -389,6 +389,53 @@ 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 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 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(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. diff --git a/roofit/roofitcore/src/RooFormula.h b/roofit/roofitcore/src/RooFormula.h index 1c3cf44181606..388582cb70b40 100644 --- a/roofit/roofitcore/src/RooFormula.h +++ b/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: diff --git a/roofit/roofitcore/src/RooFormulaVar.cxx b/roofit/roofitcore/src/RooFormulaVar.cxx index b1e19db40f145..2e00d233dcaac 100644 --- a/roofit/roofitcore/src/RooFormulaVar.cxx +++ b/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(); } } @@ -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); } diff --git a/roofit/roofitcore/test/testRooFormula.cxx b/roofit/roofitcore/test/testRooFormula.cxx index b250dbc23d953..a54870fd9df42 100644 --- a/roofit/roofitcore/test/testRooFormula.cxx +++ b/roofit/roofitcore/test/testRooFormula.cxx @@ -2,11 +2,13 @@ // Authors: Stephan Hageboeck, CERN 2020 // Jonas Rembser, CERN 2023 // Andrea Germinario, CERN 2025 +#include #include "../src/RooFormula.h" #include #include #include +#include #include @@ -92,6 +94,30 @@ 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(w2->function("f")); + + // If @2 still maps to c, changing c updates f = a*c + d = 2*5 + 4 = 14. + static_cast(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