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] RooFit workspace work differently with pyROOT in 6.32-rc1 version and 6.30/02 #15479

Closed
1 task
yuhao-wang-nju opened this issue May 10, 2024 · 4 comments
Closed
1 task

Comments

@yuhao-wang-nju
Copy link

yuhao-wang-nju commented May 10, 2024

Check duplicate issues.

  • Checked for duplicates

Description

Hi root team,

We are the ATLAS VHbbcc analysis team and we are testing with the new root preview version v6.32-rc1 since it has the bug fix related to the issue: https://root-forum.cern.ch/t/cling-jit-session-error-cannot-allocate-memory/56744

However we met some different behavior in this version compared to the previous 6.32/02 that we use.

  1. The pyROOT, we got a different type when access the RooCategory allowedState name. In the new version, it returns cppyy.gbl.std.string object, previously it is a python str object. May I ask it is designed or a bug?
  2. We found this because it crushes when we tried to pass it to re.findall() function. But according to the cppyy document, the cppyy should convert cppyy.gbl.std.string to str automatically when the str is needed. So what is the reason for not being converted automatically?
  3. We also found that it takes much longer time to run the fit using the new root version, in our case, a 1-hour fit now takes 3-hours to finish. Is there any hint what may be the cause?

Thanks a lot for your time

Yuhao

Reproducer

import ROOT

g = ROOT.TFile.Open("/afs/cern.ch/work/y/yuhao/public/test.root")
ws=g.combined
simpdf=ws.pdf("simPdf")
channelCat = simpdf.indexCat()
print(type(channelCat.begin().first))

ROOT version

6.32-rc1

Installation method

build from source

Operating system

Linux el9

Additional context

No response

@yuhao-wang-nju yuhao-wang-nju changed the title RooFit workspace work differently in 6.32-rc1 version and 6.30/02 RooFit workspace work differently with pyROOT in 6.32-rc1 version and 6.30/02 May 10, 2024
@guitargeek guitargeek self-assigned this May 10, 2024
@guitargeek
Copy link
Contributor

Hi @yuhao-wang-nju, thanks for your post!

  1. This is by design (see cppyy.gbl.std.string representation changed in ROOT master #15153)
  2. Where does the doc say that? I think cppyy pythonizes the cppyy.gbl.std.string such that it behaves like a Python str, therefore it can be used in many contexts where one would use str. But this doesn't always help. The type is different after all, and there is no automatic conversion for the reasons outlined in cppyy.gbl.std.string representation changed in ROOT master #15153. You will have to do this yourself with str() around the std::string.
  3. The evaluation was completely replaced in 6.32. Probably you have one of the corner cases where the new backend is slower and I need to debug this. Is it also possible to reproduce this slow fit with the workspace you shared? Maybe you can share the full script?

Thanks a lot!

Related PR: #14237

@guitargeek guitargeek changed the title RooFit workspace work differently with pyROOT in 6.32-rc1 version and 6.30/02 [RF] RooFit workspace work differently with pyROOT in 6.32-rc1 version and 6.30/02 May 11, 2024
@yuhao-wang-nju
Copy link
Author

yuhao-wang-nju commented May 14, 2024

Hi @guitargeek , thanks a lot for the reply and sorry for the late message.

For the first and second point, thanks a lot for the explanation. I may misunderstood the way cppyy works previously, now it seems quite reasonable to me!

For the third one, I prepared a workspace and also attached the code that we use (The dataset named obsData contained in the file is an asimov one). We are using a framework named WSMaker and I just picked up the fitting code. It is a bit long and hope it won't bring too much trouble to you. Here is the command we use for fit:
root -l -b -q '$WORKDIR/FitCrossCheckForLimits.C+(FitToAsimov,1.0,0.0,true,"ws.root","output/","combined","ModelConfig","obsData",false,false)'

The file is a bit large so here is the gdrive link. Thanks a lot in advance for your time!

@yuhao-wang-nju
Copy link
Author

After the official release, we tested and found there is no slowdown of the fit. Thanks a lot for the updates and reply! I think we can close the issue now;)

@dpiparo dpiparo added this to Issues in Fixed in: not applicable via automation Jun 1, 2024
@dpiparo
Copy link
Member

dpiparo commented Jun 1, 2024

Thanks for reporting and retesting.

@dpiparo dpiparo closed this as completed Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants