[RF] Make RooFitResult set methods public #12017

will-cern · 2023-01-12T12:32:57Z

This allows non-friends to construct RooFitResults, which allows for the removal of protected->public override in xRooFit for this class.

phsft-bot · 2023-01-12T12:33:00Z

Can one of the admins verify this patch?

bellenot · 2023-01-12T13:02:27Z

@phsft-bot build

phsft-bot · 2023-01-12T13:02:33Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

roofit/xroofit/src/xRooFit.cxx

roofit/roofitcore/inc/RooFitResult.h

guitargeek

Hi @will-cern, thank you very much for this PR!

I have made a few comments inline.

In general, is it possible to have the following functions still protected?

  void fillCorrMatrix() ;
   void fillLegacyCorrMatrix() const ;
   void fillPrefitCorrMatrix();

   double correlation(Int_t row, Int_t col) const;
   double covariance(Int_t row, Int_t col) const;

The first three are implementation details and don't make sense to be called by anyone I think, right? They don't even take input arguments.

And the final two are not needed because you can just do things like correlationMatrix()(row, col) instead.

guitargeek

Sorry, pushed the wrong button. I request changes of course.

Also, please remember to code-format the xRooFit.cxx file after implementing the suggestions 👍

phsft-bot · 2023-01-12T13:36:23Z

Build failed on mac12/noimt.
Running on macphsft17.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Warnings:

[2023-01-12T13:11:48.604Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/core/base/src/TDirectory.cxx:1245:7: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

Failing tests:

And 63 more

will-cern · 2023-01-12T13:57:21Z

Thanks for the review - I think all responded to now

phsft-bot · 2023-01-12T14:15:52Z

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

Errors:

[2023-01-12T14:15:31.306Z] /data/sftnight/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:647:51: error: ‘void RooFitResult::fillCorrMatrix(const std::vector<double>&, const TMatrixDSym&, const TMatrixDSym&)’ is protected within this context
[2023-01-12T14:15:31.306Z] /data/sftnight/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:652:43: error: ‘void RooFitResult::setStatusHistory(std::vector<std::pair<std::__cxx11::basic_string<char>, int> >&)’ is protected within this context
[2023-01-12T14:15:31.626Z] /data/sftnight/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:922:45: error: ‘void RooFitResult::fillCorrMatrix(const std::vector<double>&, const TMatrixDSym&, const TMatrixDSym&)’ is protected within this context

phsft-bot · 2023-01-12T14:44:56Z

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

Errors:

[2023-01-12T14:44:52.720Z] FAILED: roofit/roofitcore/CMakeFiles/RooFitCore.dir/src/RooMinimizer.cxx.o
[2023-01-12T14:44:54.105Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:647:15: error: 'fillCorrMatrix' is a protected member of 'RooFitResult'
[2023-01-12T14:44:54.105Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:652:12: error: 'setStatusHistory' is a protected member of 'RooFitResult'
[2023-01-12T14:44:54.105Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:922:9: error: 'fillCorrMatrix' is a protected member of 'RooFitResult'

phsft-bot · 2023-01-12T14:46:20Z

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

Errors:

[2023-01-12T14:45:45.656Z] FAILED: roofit/roofitcore/CMakeFiles/RooFitCore.dir/src/RooMinimizer.cxx.o
[2023-01-12T14:45:46.689Z] /home/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:647:51: error: ‘void RooFitResult::fillCorrMatrix(const std::vector<double>&, const TMatrixDSym&, const TMatrixDSym&)’ is protected within this context
[2023-01-12T14:45:46.689Z] /home/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:652:43: error: ‘void RooFitResult::setStatusHistory(std::vector<std::pair<std::__cxx11::basic_string<char>, int> >&)’ is protected within this context
[2023-01-12T14:45:46.689Z] /home/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooMinimizer.cxx:922:45: error: ‘void RooFitResult::fillCorrMatrix(const std::vector<double>&, const TMatrixDSym&, const TMatrixDSym&)’ is protected within this context

roofit/xroofit/src/xRooFit.cxx

guitargeek

Hi Will, thanks for the updates!

I just have a few more inline comment. I also tried to compile the code on my machine, and I got a warning in xRooFit line 508 because of the unused bool save = true; variable. can you please remove this line?

Also, sorry for not being clear earlier, but when I asked to make the fillCorrMatrix() protected again, I only meant the overload that doesn't take parameters. Also, you made setStatusHistory protected now, but it should be public.

In other words, these two overloads need to be made public now:

  void fillCorrMatrix(const std::vector<double>& globalCC, const TMatrixDSym& corrs, const TMatrixDSym& covs) ;
  void setStatusHistory(std::vector<std::pair<std::string,int> >& hist) { _statusHistory = hist ; }

One more thing: can you please squash the commit history such that there are again only two commits, one for xroofit and one for roofit? We don't need all the commits for addressing the review comments. Normally I would squash-on-merge, but then I can only squash to one single commit. And it would be nice to keep the history of roofit and xroofit separate.

will-cern · 2023-01-12T15:25:21Z

yes this was my fault for not checking it compiled first before pushing, I should have spotted this!

phsft-bot · 2023-01-12T15:31:29Z

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

guitargeek · 2023-01-12T16:02:55Z

@phsft-bot build

phsft-bot · 2023-01-12T16:03:02Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2023-01-12T16:10:07Z

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

Errors:

[2023-01-12T16:10:06.262Z] LINK : fatal error LNK1104: cannot open file 'C:\build\workspace\root-pullrequests-build\build\bin\libCore.dll' [C:\build\workspace\root-pullrequests-build\build\core\Core.vcxproj]

…hods

guitargeek · 2023-01-12T16:25:40Z

@phsft-bot build

phsft-bot · 2023-01-12T16:25:46Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2023-01-12T16:31:34Z

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

Errors:

[2023-01-12T16:31:32.452Z] LINK : fatal error LNK1104: cannot open file 'C:\build\workspace\root-pullrequests-build\build\bin\libCore.dll' [C:\build\workspace\root-pullrequests-build\build\core\Core.vcxproj]

phsft-bot · 2023-01-12T16:55:22Z

Build failed on mac12/noimt.
Running on macphsft17.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Warnings:

[2023-01-12T16:31:33.816Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/core/base/src/TDirectory.cxx:1245:7: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

Failing tests:

And 63 more

will-cern requested review from guitargeek, hageboeck and lmoneta as code owners January 12, 2023 12:32

bellenot assigned guitargeek Jan 12, 2023

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/roofitcore/inc/RooFitResult.h Outdated Show resolved Hide resolved

guitargeek approved these changes Jan 12, 2023

View reviewed changes

guitargeek added in:RooFit/RooStats improvement labels Jan 12, 2023

guitargeek requested changes Jan 12, 2023

View reviewed changes

guitargeek changed the title ~~make RooFitResult set methods public~~ [RF] Make RooFitResult set methods public Jan 12, 2023

guitargeek mentioned this pull request Jan 12, 2023

[RF] RooFit open issues and backports for 6.28.00 release #11856

Closed

52 tasks

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek reviewed Jan 12, 2023

View reviewed changes

roofit/xroofit/src/xRooFit.cxx Outdated Show resolved Hide resolved

guitargeek requested changes Jan 12, 2023

View reviewed changes

will-cern force-pushed the RooFitResult_publicMethods branch 2 times, most recently from b5b2f77 to eeb1fdf Compare January 12, 2023 15:48

will-cern added 2 commits January 12, 2023 16:21

make various set methods public

c70f45b

remove define protected->public of RooFitResult, given new public met…

203909d

…hods

will-cern force-pushed the RooFitResult_publicMethods branch from eeb1fdf to 203909d Compare January 12, 2023 16:22

guitargeek approved these changes Jan 12, 2023

View reviewed changes

guitargeek merged commit 2966932 into root-project:master Jan 12, 2023

guitargeek mentioned this pull request Jan 18, 2023

[v628][RF] Backports of RooFit PRs to v6-28-00-patches: Part 5 #12057

Merged

[RF] Make RooFitResult set methods public #12017

[RF] Make RooFitResult set methods public #12017

Uh oh!

Conversation

will-cern commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Uh oh!

bellenot commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Jan 12, 2023

Warnings:

Failing tests:

Uh oh!

will-cern commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Errors:

Uh oh!

phsft-bot commented Jan 12, 2023

Errors:

Uh oh!

phsft-bot commented Jan 12, 2023

Errors:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

will-cern commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Uh oh!

guitargeek commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Errors:

Uh oh!

guitargeek commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Uh oh!

phsft-bot commented Jan 12, 2023

Errors:

Uh oh!

phsft-bot commented Jan 12, 2023

Warnings:

Failing tests:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants