Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions roofit/roofitcore/src/RooFormula.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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<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.
Expand Down
1 change: 1 addition & 0 deletions roofit/roofitcore/src/RooFormula.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
8 changes: 4 additions & 4 deletions roofit/roofitcore/src/RooFormulaVar.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Expand All @@ -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());
}
}
Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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);
}

Expand Down
26 changes: 26 additions & 0 deletions roofit/roofitcore/test/testRooFormula.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -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<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
Expand Down