Skip to content

[RF] Address the RooFormulaVar and RooGenericPdf re-indexing issue during serialization#22850

Open
tekinertekin wants to merge 1 commit into
root-project:masterfrom
tekinertekin:rooformulavar-reindex-21371
Open

[RF] Address the RooFormulaVar and RooGenericPdf re-indexing issue during serialization#22850
tekinertekin wants to merge 1 commit into
root-project:masterfrom
tekinertekin:rooformulavar-reindex-21371

Conversation

@tekinertekin

Copy link
Copy Markdown

This Pull request:

RooFormulaVar @n indices were inconsistent after a write-read cycle in cases where some of the unused parameters have been trimmed, causing incorrect results. We now use the method RooFormula::reindexedFormulaForUsedVars(), which re-indexes the saved formula expression to the position of the usedVariables(), in the 4 spots where _formExpr is being set (both constructors, copy constructor and redirectServersHook). The code was authored by me. Claude Opus 4.8 assisted with reproducing the bug, local builds/tests, and reviewing the code — it did not generate any of the code. This is the binary/TFile equivalent of the same bug fixed in #17291; it came from there because that path wasn't tested — this PR closes that gap.

Added a regression test RooFormula.SerializationWithUnusedParam.

Fix For:

Fixes #21371

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #21371

@guitargeek guitargeek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! Thanks a lot for the fixes. Can you give the RooGenericPdf the same treatment in the same commit? It's analogous to RooFormulaVar, just for pdfs instead of functions.

Also, you missed to replace one place where formulaString() needed to be replaced in RooFormulaVar.cxx (around line 148).

After these two improvements, the PR would look good to me (also the CI should be green 🙂). Thanks!

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

Test Results

    22 files      22 suites   3d 13h 56m 26s ⏱️
 3 871 tests  3 870 ✅ 0 💤 1 ❌
76 014 runs  76 013 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 7c58b80.

♻️ This comment has been updated with latest results.

@tekinertekin
tekinertekin force-pushed the rooformulavar-reindex-21371 branch from f159d6f to b941fae Compare July 23, 2026 13:35
@tekinertekin tekinertekin changed the title [RF] Address the RooFormulaVar re-indexing issue during serialization [RF] Address the RooFormulaVar and RooGenericPdf re-indexing issue during serialization Jul 23, 2026
@tekinertekin
tekinertekin force-pushed the rooformulavar-reindex-21371 branch from b941fae to 7c58b80 Compare July 23, 2026 13:52
@tekinertekin

Copy link
Copy Markdown
Author

Thanks a lot for the review. Addressed both points in the same commit — RooGenericPdf now gets the same re-indexing treatment, and I fixed the missed formulaString() in RooFormulaVar::getFormula() (line 148). Also added a GenericPdf.SerializationWithUnusedParam regression test and rebased onto current master to resolve the conflict. clang-format is clean locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RooFormulaVar @N parameter indices silently shift after workspace serialization

3 participants