Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RF] Parameter ordering bug in RooFormulaArgStreamer #17291

Closed
1 task done
mark-owen opened this issue Dec 17, 2024 · 0 comments · Fixed by #17292
Closed
1 task done

[RF] Parameter ordering bug in RooFormulaArgStreamer #17291

mark-owen opened this issue Dec 17, 2024 · 0 comments · Fixed by #17292

Comments

@mark-owen
Copy link

mark-owen commented Dec 17, 2024

Check duplicate issues.

  • Checked for duplicates

Description

I noticed that if the RooFormulaArgStreamer is used with a RooFormulaVar that is built with the syntax like:
RooFormulaVar("name","title","@1+@2+@3","RooArgList(arg1,arg2,arg2)")
then in the case there are more than 10 arguments it can happen that the exported JSON has the wrong parameter names. I believe this is because the logic at 548-568 in https://root.cern.ch/doc/v634/JSONFactories__RooFitCore_8cxx_source.html assumes that the vector paramsWithIndex is ordered by the integer, while the sort actually orders on the pointers in the vector. In the case where the order of the pointers doesn't match to integer, this can cause the ReplaceAll at 567 to act more than once on the same parameter. Below is a minimal bit of code reproducing the problem.

I think it is fixed by removing the vector<std::pair<RooAbsArg *, std::size_t>> and just directly looping (in reverse order) on the parameters of the formula. I created a pull request to fix.

Reproducer

Put the following code in a macro and run as python macro.C:

import ROOT
import random

# create a large number of variables
nvar = 67
vs = []
for i in range(0, nvar):
    v = ROOT.RooRealVar("v" + str(i), "v" + str(i), 0.0, -5.0, 5.0)
    vs.append(v)

# shuffle the order - if you comment this the code works fine
random.seed(871)
random.shuffle(vs)

# make a formula var that is product of the variables
thestring = ""
arglist = ROOT.RooArgList()
for i, v in enumerate(vs):
    thestring = thestring + "@" + str(i) + "*"
    arglist.add(v)

thestring = thestring.rstrip('*')
fv = ROOT.RooFormulaVar("fv", "fv", thestring, arglist)

# make gaussian with mean as product of the variables and import into workspace
x = ROOT.RooRealVar("x","x",0.0,-5.0,5.0)
g = ROOT.RooGaussian("g","g",x, fv, ROOT.RooRealConstant.value(1.0))
ws = ROOT.RooWorkspace("ws")
ws.Import(g)
print(fv.expression())

# export to json
t = ROOT.RooJSONFactoryWSTool(ws)
t.exportJSON('fv.json')

# try to import, crashes because the export was bad
newws = ROOT.RooWorkspace("newws")
t2 = ROOT.RooJSONFactoryWSTool(newws)
t2.importJSON('fv.json')
print(newws.obj('fv').expression())

ROOT version

/cvmfs/atlas.cern.ch/repo/sw/software/0.4/StatAnalysis/0.4.1/InstallArea/x86_64-el9-gcc13-opt/bin/root

6.32.02

Installation method

CVMFS installation

Operating system

Linux

Additional context

No response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants