[RF] Address the RooFormulaVar and RooGenericPdf re-indexing issue during serialization#22850
[RF] Address the RooFormulaVar and RooGenericPdf re-indexing issue during serialization#22850tekinertekin wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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!
Test Results 22 files 22 suites 3d 13h 56m 26s ⏱️ For more details on these failures, see this check. Results for commit 7c58b80. ♻️ This comment has been updated with latest results. |
f159d6f to
b941fae
Compare
…ring serialization
b941fae to
7c58b80
Compare
|
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. |
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:
This PR fixes #21371