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] Support ShapeSys in JSON tool #9683

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Conversation

gartrog
Copy link
Contributor

@gartrog gartrog commented Jan 25, 2022

Refactor ParamHistFunc treatment to support both MC stat (BB-lite)
and user-defined ShapeSys

Validated on ATLAS VHbb workspace

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@gartrog
Copy link
Contributor Author

gartrog commented Jan 25, 2022

Tagging @cburgard for review

@cburgard
Copy link
Contributor

Looks good to me. Only one small suggestion: The createPHF function (which was originally written to only deal with mcstat) has been generalized by you to also work for ShapeSys, which is great. I think it would be good if the parameter names would reflect that (i.e., rename statErrorType to constraintType or similar).

In addition to that, I'm sure @guitargeek will request you run clang-format -i --style=file roofit/hs3/src/*.cxx.

Thanks a lot for going the extra mile of adding this!

@gartrog
Copy link
Contributor Author

gartrog commented Jan 25, 2022

Done.

@guitargeek
Copy link
Contributor

Hi @gartrog, thanks for the PR! I see that it collides with my memory leak PR: #9690

Is it okay if I merge the memory management PR first, and then we come back to this?

@gartrog
Copy link
Contributor Author

gartrog commented Jan 25, 2022

Hi @guitargeek yes please merge the other one first. It should be easy to adapt this one afterwards.

@guitargeek
Copy link
Contributor

Ok! The conflicting PR has now been merged.

Refactor ParamHistFunc treatment to support both MC stat (BB-lite)
and user-defined ShapeSys

Validated on ATLAS VHbb workspace
@gartrog
Copy link
Contributor Author

gartrog commented Jan 27, 2022

It should now be ok, @guitargeek

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

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
Copy link
Collaborator

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

Warnings:

  • [2022-01-28T11:36:34.387Z] /Volumes/HDD2/build/workspace/root-pullrequests-build/root/roofit/hs3/src/RooJSONFactoryWSTool.cxx:498:21: warning: loop variable 'absv' is always a copy because the range of type 'ROOT::RRangeCast<RooRealVar *, false, const RooArgList &>' does not return a reference [-Wrange-loop-analysis]

Co-authored-by: Jonas Rembser <jonas.rembser@cern.ch>
@phsft-bot
Copy link
Collaborator

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

Warnings:

  • [2022-01-28T16:31:21.841Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on 'RooFit::Experimental::JSONNode::child_iterator_t<RooFit::Experimental::JSONNode>::Impl' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:21.841Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on 'RooFit::Experimental::JSONNode::child_iterator_t<const RooFit::Experimental::JSONNode>::Impl' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:21.841Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on non-final '(anonymous namespace)::childItImpl<RooFit::Experimental::JSONNode>' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:21.841Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on non-final '(anonymous namespace)::childItImpl<const RooFit::Experimental::JSONNode>' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:26.260Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on non-final 'TJSONTree::Node::childItImpl<RooFit::Experimental::JSONNode, TJSONTree::Node, nlohmann::detail::iter_impl<nlohmann::basic_json<>>>' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:26.542Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on 'RooFit::Experimental::JSONNode::child_iterator_t<RooFit::Experimental::JSONNode>::Impl' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:26.542Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on non-final 'TJSONTree::Node::childItImpl<const RooFit::Experimental::JSONNode, const TJSONTree::Node, nlohmann::detail::iter_impl<const nlohmann::basic_json<>>>' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:26.542Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on 'RooFit::Experimental::JSONNode::child_iterator_t<const RooFit::Experimental::JSONNode>::Impl' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:28.071Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on 'RooFit::Experimental::JSONNode::child_iterator_t<const RooFit::Experimental::JSONNode>::Impl' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
  • [2022-01-28T16:31:29.441Z] /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/usr/include/c++/v1/memory:2084:5: warning: delete called on 'RooFit::Experimental::JSONNode::child_iterator_t<const RooFit::Experimental::JSONNode>::Impl' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]

And 1 more

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you very much for implementing this!

The compiler warnings seen in the CI tests will be fixed by #9734.

@guitargeek guitargeek merged commit cd514f9 into root-project:master Jan 28, 2022
@phsft-bot
Copy link
Collaborator

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

Errors:

  • [2022-01-28T23:10:56.648Z] stderr: error: Failed to merge in the changes.
  • [2022-01-28T23:13:33.547Z] CMake Error at C:/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1064 (message):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants