[RF] Fixes evaluate() function in RooRealL #9456

Zeff020 · 2021-12-16T18:35:25Z

This Pull request:

Changes or fixes:

The evaluate() function on the RooFit::TestStatistics::RooRealL class was not functional. This PR fixes it by adding a RooArgSet member to RooRealL called vars_obs_ which initially contains the same variables as the RooArgProxy and ensures that the proxy variables are properly transferred to the internal variables of the likelihood (contained in the RooArgSet vars_obs_) before evaluation.

This change was made with the help of @wverkerke

Checklist:

tested changes locally
updated the docs (if necessary)

This PR fixes #

phsft-bot · 2021-12-16T18:35:28Z

Can one of the admins verify this patch?

egpbos · 2021-12-16T19:08:45Z

@guitargeek This is a redo of #9402, now rebased on master.

guitargeek · 2021-12-16T19:18:06Z

@phsft-bot build

phsft-bot · 2021-12-16T19:18:12Z

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

phsft-bot · 2021-12-16T19:47:07Z

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

Failing tests:

projectroot.test.test_stressroofit_interpreted

phsft-bot · 2021-12-16T19:47:51Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.test.test_stressroofit_interpreted

phsft-bot · 2021-12-16T19:50:15Z

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-12-16T19:51:43Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.test.test_stressroofit_interpreted

phsft-bot · 2021-12-16T20:49:14Z

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

Failing tests:

phsft-bot · 2021-12-16T22:25:21Z

Build failed on mac1015/python3.
Running on macitois22.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.test.test_stressroofit_interpreted

phsft-bot · 2021-12-17T02:59:49Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Zeff020 · 2021-12-17T07:44:13Z

I see why it failed, that stressRooFit test that failed was passed on my pc so when updating the reference file with all the plots the relevant plot was not included. I will add it manually and commit

Axel-Naumann · 2021-12-17T14:21:14Z

Is test/stressRooFit_ref.root meant to end up on https://root.cern? If so, should the file be removed from this PR?

Zeff020 · 2021-12-17T14:54:50Z

@Axel-Naumann The file was already there, I just modified it. I am not sure how it would end up on https://root.cern.

Axel-Naumann · 2021-12-17T16:16:05Z

Sorry, I confused this file with a different RooFit test file, all good!

Zeff020 · 2021-12-17T17:39:36Z

No problem!

@guitargeek I fixed the bug that was causing the tests to fail

egpbos · 2021-12-20T09:32:25Z

@phsft-bot build

phsft-bot · 2021-12-20T09:32:31Z

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

phsft-bot · 2021-12-20T09:34:30Z

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

Errors:

[2021-12-20T09:34:23.358Z] stderr: error: Failed to merge in the changes.
[2021-12-20T09:34:29.686Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2021-12-20T09:34:39Z

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

Errors:

[2021-12-20T09:34:38.527Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2021-12-20T09:34:40Z

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

Errors:

[2021-12-20T09:34:38.941Z] CMake Error at /data/sftnight/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2021-12-20T09:35:17Z

Build failed on mac1015/python3.
Running on macitois19.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-12-20T09:35:10.761Z] stderr: error: Failed to merge in the changes.
[2021-12-20T09:35:15.945Z] CMake Error at /Volumes/HDD2/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2021-12-20T09:36:16Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-12-20T09:36:08.367Z] stderr: error: Failed to merge in the changes.
[2021-12-20T09:36:15.074Z] CMake Error at /mnt/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2021-12-20T09:41:23Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-12-20T09:41:17.499Z] stderr: error: Failed to merge in the changes.
[2021-12-20T09:41:23.141Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

guitargeek · 2022-01-19T22:58:43Z

test/stressRooFit_tests.h

@@ -6456,4 +6456,4 @@ class TestBasic804 : public RooUnitTest

  return kTRUE ;
  }
-} ;
+} ;


You don't need to touch that file.

roofit/roofitcore/test/TestStatistics/testPlot.cpp

roofit/roofitcore/test/CMakeLists.txt

roofit/roofitcore/test/TestStatistics/testPlot.cpp

guitargeek · 2022-01-19T23:15:48Z

Hi @Zeff020, thanks for the quick followup! I have made a few more change requests, they are a bit minute but I hope to teach you some of the good practices we use here for the future :)

Also, if you introduce any new source file to RooFit, please format it with clang-format using the official ROOT style file. You will see that the right style was used if your sources are indented with three spaces. In this PR, this testPlot.cpp should be formatted.

When you have done the formatting and implemented my comments, we can test and merge.

guitargeek · 2022-01-19T23:16:29Z

@phsft-bot build

phsft-bot · 2022-01-19T23:16:36Z

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

phsft-bot · 2022-01-20T00:29:38Z

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

Failing tests:

And 4 more

guitargeek · 2022-01-20T10:50:12Z

@phsft-bot build

phsft-bot · 2022-01-20T10:50:18Z

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

phsft-bot · 2022-01-20T12:08:48Z

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

Failing tests:

And 4 more

guitargeek

LGTM, thanks for the PR and addressing the review comments!

guitargeek · 2022-01-20T13:06:01Z

roofit/roofitcore/test/TestStatistics/testPlot.cpp

+      std::shared_ptr<RooFit::TestStatistics::RooRealL> likelihood_real(
+         new RooFit::TestStatistics::RooRealL("likelihood", "", likelihood));


Suggested change

std::shared_ptr<RooFit::TestStatistics::RooRealL> likelihood_real(

new RooFit::TestStatistics::RooRealL("likelihood", "", likelihood));

auto likelihood_real = std::make_shared<RooFit::TestStatistics::RooRealL>("likelihood", "", likelihood);

Just for the next time: you should use make_shared to create objects directly wrapped by shared pointers, and you can use auto to automatically deduce type when it's clear to the reader what the type is anyway (here it would be clear because of the type in make_shared).

I see, will do!

phsft-bot · 2022-01-20T13:16:36Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2022-01-20T13:15:54.707Z] stderr: error: Failed to merge in the changes.
[2022-01-20T13:16:35.415Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-20T14:51:51Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-01-20T14:51:45.743Z] stderr: error: Failed to merge in the changes.
[2022-01-20T14:51:50.918Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-26T18:44:08Z

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

phsft-bot · 2022-01-26T18:45:54Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-01-26T18:45:42.278Z] stderr: error: Failed to merge in the changes.
[2022-01-26T18:45:53.527Z] CMake Error at /mnt/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-26T19:27:49Z

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

Errors:

[2022-01-26T19:27:41.254Z] stderr: error: Failed to merge in the changes.
[2022-01-26T19:27:48.924Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-26T20:17:43Z

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

Errors:

[2022-01-26T20:17:42.482Z] CMake Error at /data/sftnight/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-27T00:07:18Z

Build failed on mac1015/python3.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-01-27T00:07:06.204Z] stderr: error: Failed to merge in the changes.
[2022-01-27T00:07:18.112Z] CMake Error at /Volumes/HD2/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-27T08:19:21Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2022-01-27T08:16:48.852Z] stderr: error: Failed to merge in the changes.
[2022-01-27T08:19:20.837Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-27T12:55:30Z

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

Errors:

[2022-01-27T12:55:29.970Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

phsft-bot · 2022-01-27T14:26:34Z

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-01-27T14:26:27.786Z] stderr: error: Failed to merge in the changes.
[2022-01-27T14:26:33.316Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

Zeff020 requested review from Axel-Naumann, guitargeek, hageboeck and lmoneta as code owners December 16, 2021 18:35

egpbos added bug in:RooFit/RooStats labels Dec 16, 2021

guitargeek reviewed Jan 19, 2022

View reviewed changes

roofit/roofitcore/test/TestStatistics/testPlot.cpp Outdated Show resolved Hide resolved

guitargeek reviewed Jan 19, 2022

View reviewed changes

roofit/roofitcore/test/CMakeLists.txt Outdated Show resolved Hide resolved

guitargeek reviewed Jan 19, 2022

View reviewed changes

roofit/roofitcore/test/TestStatistics/testPlot.cpp Outdated Show resolved Hide resolved

Formatted with clang-format, implemented suggestions

fea1bd4

guitargeek approved these changes Jan 20, 2022

View reviewed changes

guitargeek reviewed Jan 20, 2022

View reviewed changes

guitargeek merged commit 9090854 into root-project:master Jan 20, 2022

rahulgrit pushed a commit to rahulgrit/root that referenced this pull request Jan 25, 2022

[RF] Fixes evaluate() function in RooRealL (root-project#9456)

0c8242b

guitargeek pushed a commit to guitargeek/root that referenced this pull request Jan 26, 2022

[RF] Fixes evaluate() function in RooRealL (root-project#9456)

6ca2259

guitargeek mentioned this pull request Jan 26, 2022

[v626][RF] Backports of RooFit PRs to v6-26-00-patches: Part 3 #9710

Merged

guitargeek changed the title ~~Fixes evaluate() function in rooRealL~~ [RF] Fixes evaluate() function in RooRealL Jan 26, 2022

guitargeek pushed a commit that referenced this pull request Jan 26, 2022

[RF] Fixes evaluate() function in RooRealL (#9456)

3aa2a14

		std::shared_ptr<RooFit::TestStatistics::RooRealL> likelihood_real(
		new RooFit::TestStatistics::RooRealL("likelihood", "", likelihood));

	std::shared_ptr<RooFit::TestStatistics::RooRealL> likelihood_real(
	new RooFit::TestStatistics::RooRealL("likelihood", "", likelihood));
	auto likelihood_real = std::make_shared<RooFit::TestStatistics::RooRealL>("likelihood", "", likelihood);

[RF] Fixes evaluate() function in RooRealL #9456

[RF] Fixes evaluate() function in RooRealL #9456

Conversation

Zeff020 commented Dec 16, 2021 • edited Loading

This Pull request:

Changes or fixes:

Checklist:

phsft-bot commented Dec 16, 2021

egpbos commented Dec 16, 2021

guitargeek commented Dec 16, 2021

phsft-bot commented Dec 16, 2021

phsft-bot commented Dec 16, 2021

Failing tests:

phsft-bot commented Dec 16, 2021

Failing tests:

phsft-bot commented Dec 16, 2021

Failing tests:

phsft-bot commented Dec 16, 2021

Failing tests:

phsft-bot commented Dec 16, 2021

Failing tests:

phsft-bot commented Dec 16, 2021

Failing tests:

phsft-bot commented Dec 17, 2021

Zeff020 commented Dec 17, 2021

Axel-Naumann commented Dec 17, 2021

Zeff020 commented Dec 17, 2021

Axel-Naumann commented Dec 17, 2021

Zeff020 commented Dec 17, 2021 • edited Loading

egpbos commented Dec 20, 2021

phsft-bot commented Dec 20, 2021

phsft-bot commented Dec 20, 2021

Errors:

phsft-bot commented Dec 20, 2021

Errors:

phsft-bot commented Dec 20, 2021

Errors:

phsft-bot commented Dec 20, 2021

Errors:

phsft-bot commented Dec 20, 2021

Errors:

phsft-bot commented Dec 20, 2021

Errors:

guitargeek Jan 19, 2022

Choose a reason for hiding this comment

guitargeek commented Jan 19, 2022

guitargeek commented Jan 19, 2022

phsft-bot commented Jan 19, 2022

phsft-bot commented Jan 20, 2022

Failing tests:

guitargeek commented Jan 20, 2022

phsft-bot commented Jan 20, 2022

phsft-bot commented Jan 20, 2022

Failing tests:

guitargeek left a comment

Choose a reason for hiding this comment

guitargeek Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Zeff020 Jan 20, 2022

Choose a reason for hiding this comment

phsft-bot commented Jan 20, 2022

Errors:

phsft-bot commented Jan 20, 2022

Errors:

phsft-bot commented Jan 26, 2022

phsft-bot commented Jan 26, 2022

Errors:

phsft-bot commented Jan 26, 2022

Errors:

phsft-bot commented Jan 26, 2022

Errors:

phsft-bot commented Jan 27, 2022

Errors:

phsft-bot commented Jan 27, 2022

Errors:

phsft-bot commented Jan 27, 2022

Errors:

phsft-bot commented Jan 27, 2022

Errors:

Zeff020 commented Dec 16, 2021 •

edited

Loading

Zeff020 commented Dec 17, 2021 •

edited

Loading

guitargeek Jan 20, 2022 •

edited

Loading