[RF] Translated rf_408_RDataFrameToRooFit.C to python by harshal0815 · Pull Request #8705 · root-project/root

harshal0815 · 2021-07-20T21:14:34Z

This Pull request:

Translated rf_408_RDataFrameToRooFit.C
Changed signature of constructor to take RooAbsArg by reference.
Changed ROOT.RooArgList in args to accept a simple Python list.

Changes or fixes:

Checklist:

tested changes locally
updated the docs (if necessary)

This PR fixes #

phsft-bot · 2021-07-20T21:14:37Z

Can one of the admins verify this patch?

guitargeek · 2021-07-21T06:58:35Z

@phsft-bot build

phsft-bot · 2021-07-21T06:58:42Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-21T07:23:55Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2021-07-21T07:38:37Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-21T07:00:28.045Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

phsft-bot · 2021-07-21T08:15:47Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

tutorials/roofit/rf313_paramranges.py

tutorials/roofit/rf902_numgenconfig.py

tutorials/roofit/rf704_amplitudefit.py

guitargeek

Hi Harshal, thanks for the PR! I requested some changes to fix the crashes.

The problem was that cppyy translated the 0 literal to a nullptr, and not to a 0.0 double as you intended in your translation. That's not good that this can get confused!

Can you do me a favor so I don't need to apply the fix myself on the C++ side? In RooAbsCollection.h, there is this RooAbsArgPtrOrDouble that can either be constructed from RooAbsArg pointer or a double, which is how the implicit call to RooConst is achieved. Can you make an additional commit in this PR to change the signature of the first constructor to take the RooAbsArg by reference, with a meaningful commit message? It would look like this:

RooAbsArgPtrOrDouble(RooAbsArg & arg) : ptr{&arg}, hasPtr{true} {}

It doesn't matter for pyROOT if the take the argument by pointer or by reference, and by taking a reference we avoid the problem with the nullptr. Thanks!

Also, please split up the existing commit into two commits: one commit for the new RDataFrame tutorial translation, and one the the RooArgList migration. Not mixing different developments in the same commit is a good habit, as it makes reviewing much easier.

guitargeek · 2021-07-21T14:30:38Z

@phsft-bot build

phsft-bot · 2021-07-21T14:30:44Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-21T15:15:50Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.runtutorials.tutorial_roofit_rf704_amplitudefit_py

phsft-bot · 2021-07-21T16:30:06Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.runtutorials.tutorial_roofit_rf704_amplitudefit_py

phsft-bot · 2021-07-21T16:34:20Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-21T15:49:21.514Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

guitargeek · 2021-07-26T06:47:13Z

@phsft-bot build

phsft-bot · 2021-07-26T06:47:19Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-26T08:00:41Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

phsft-bot · 2021-07-26T08:07:21Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

guitargeek

Thanks for the contribution! This PR:

fixes a problem with the implicit creation of RooFit collections from python collections, where the integer zero is now interpreted as a RooConst and not as a nullptr
migrates the RooFit pyROOT tutorials to use python lists instead of RooArgList
adds a translation of the rf408_RDataFrameToRooFit tutorial to pyROOT, meaning we have now all RooFit tutorials translated and the corresponding GitHub issues can be closed
the failures in the CI are unrelated to this PR

harshal0815 requested a review from couet as a code owner July 20, 2021 21:14

jblomer assigned guitargeek Jul 21, 2021

guitargeek reviewed Jul 21, 2021

View reviewed changes

tutorials/roofit/rf313_paramranges.py Outdated Show resolved Hide resolved

guitargeek reviewed Jul 21, 2021

View reviewed changes

tutorials/roofit/rf902_numgenconfig.py Outdated Show resolved Hide resolved

guitargeek reviewed Jul 21, 2021

View reviewed changes

tutorials/roofit/rf704_amplitudefit.py Outdated Show resolved Hide resolved

guitargeek requested changes Jul 21, 2021

View reviewed changes

harshal0815 added 2 commits July 21, 2021 19:11

[RF] Translated rf_408_RDataFrameToRooFit.C to python

f9ab68e

[RF] Changed signature of constructor to take RooAbsArg by reference.

9c40b3a

harshal0815 requested review from hageboeck and lmoneta as code owners July 21, 2021 13:49

guitargeek added in:RooFit in:Python Interface labels Jul 22, 2021

[RF] Changed ROOT.RooArgList in args to accept a simple Python list

bbab2bf

guitargeek approved these changes Jul 26, 2021

View reviewed changes

guitargeek merged commit 28c826d into root-project:master Jul 26, 2021

guitargeek linked an issue Jul 26, 2021 that may be closed by this pull request

[RF] Translate all RooFit tutorials to Python #8523

Closed

Conversation

harshal0815 commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This Pull request:

Changes or fixes:

Checklist:

Uh oh!

phsft-bot commented Jul 20, 2021

Uh oh!

guitargeek commented Jul 21, 2021

Uh oh!

phsft-bot commented Jul 21, 2021

Uh oh!

phsft-bot commented Jul 21, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 21, 2021

Errors:

Failing tests:

Uh oh!

phsft-bot commented Jul 21, 2021

Failing tests:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guitargeek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitargeek commented Jul 21, 2021

Uh oh!

phsft-bot commented Jul 21, 2021

Uh oh!

phsft-bot commented Jul 21, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 21, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 21, 2021

Errors:

Failing tests:

Uh oh!

guitargeek commented Jul 26, 2021

Uh oh!

phsft-bot commented Jul 26, 2021

Uh oh!

phsft-bot commented Jul 26, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 26, 2021

Failing tests:

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

harshal0815 commented Jul 20, 2021 •

edited

Loading

guitargeek left a comment •

edited

Loading