Skip to content

Conversation

@hageboeck
Copy link
Member

@hageboeck hageboeck commented Apr 30, 2020

After giving RooFit's categories a new interface, a lot of legacy code was left in place - the new categories support both the old and new interface.
The PR was split in two parts partly for testing that old code would still work, partly to make #5502 smaller.
Here, the big cleanup happens:

  • Most uses of the legacy interface are replaced with the new interface
  • Now-unused classes are removed.

@hageboeck hageboeck requested review from eguiraud and lmoneta April 30, 2020 10:28
@hageboeck hageboeck self-assigned this Apr 30, 2020
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot

This comment has been minimized.

@hageboeck hageboeck force-pushed the cleanupCategories branch from addda31 to 6b55e31 Compare May 4, 2020 08:54
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot

This comment has been minimized.

@hageboeck hageboeck force-pushed the cleanupCategories branch from 6b55e31 to ca36c00 Compare May 4, 2020 11:09
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@hageboeck hageboeck force-pushed the cleanupCategories branch from ca36c00 to b60341f Compare May 4, 2020 11:14
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@hageboeck hageboeck force-pushed the cleanupCategories branch from b60341f to ad20740 Compare May 4, 2020 18:16
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

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

Failing tests:

@hageboeck hageboeck force-pushed the cleanupCategories branch from ad20740 to 7367bb0 Compare May 5, 2020 06:04
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@hageboeck hageboeck marked this pull request as ready for review May 5, 2020 06:05
hageboeck added 7 commits May 5, 2020 08:09
Provide overloads for setting category states from integers, const char*
and std::string, as well as from other category states.
Remove explicit uses of the deprecated RooCatType inside roofit.
When setting R__LESS_INCLUDES, RooAbsCategory.h stops to automatically
include the deprectated RooCatType.h. Here, all explicit uses of this
class and header are removed.
Only a few classes that weren't easy to update still use it, but these
include it directly as `RooFitLegacy/RooCatTypeLegacy.h`.
Make use of the fact that the new category interface also accepts
std::string.
Put updated category interface demos in tutorials.
Also extend rf404 with instructions on how to query, iterate and set states.
The deprecated RooAbsCategory::lookupType() is sometimes used to check
if a string label refers to a valid category state.
It returns a pointer to the deprecated RooCatType, but often, the
pointer was only checked if it's nullptr. These instances have been
replaced with RooAbsCategory::hasLabel().
@hageboeck hageboeck force-pushed the cleanupCategories branch from 7367bb0 to 6c66dd6 Compare May 5, 2020 06:10
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

@hageboeck hageboeck removed request for couet and oshadura May 5, 2020 06:16
Copy link
Contributor

@eguiraud eguiraud left a comment

Choose a reason for hiding this comment

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

LGTM, many nice small improvements.

IIUC RooAbsCollection offers some STL compatibility support, namely begin, end, rbegin and rend. At this point it might be good to decide if it should satisfy all of the STL container requirements or not. It's a bit misleading to only offer part of the interface expected from an STL container.

@hageboeck
Copy link
Member Author

LGTM, many nice small improvements.

IIUC RooAbsCollection offers some STL compatibility support, namely begin, end, rbegin and rend. At this point it might be good to decide if it should satisfy all of the STL container requirements or not. It's a bit misleading to only offer part of the interface expected from an STL container.

Point taken. This is now:
https://sft.its.cern.ch/jira/browse/ROOT-10750

@hageboeck hageboeck added this to the 6.22 milestone May 5, 2020
hageboeck pushed a commit to hageboeck/root that referenced this pull request May 5, 2020
Amended to not revert the changes to be commit in PR root-project#5514.
hageboeck pushed a commit to hageboeck/root that referenced this pull request May 5, 2020
Amended to not revert the changes to be commit in PR root-project#5514.
hageboeck pushed a commit to hageboeck/root that referenced this pull request May 5, 2020
Amended to not revert the changes to be added in PR root-project#5514.
@phsft-bot
Copy link

Build failed on ROOT-fedora29/python3.
Running on root-fedora29-3.cern.ch:/build/workspace/root-pullrequests-build
See cdash.
See console output.

Failing tests:

@hageboeck hageboeck merged commit e06e4ae into root-project:master May 5, 2020
@hageboeck hageboeck deleted the cleanupCategories branch May 5, 2020 13:01
hageboeck pushed a commit to hageboeck/root that referenced this pull request May 5, 2020
Amended to not revert the changes to be added in PR root-project#5514.
linev added a commit that referenced this pull request May 5, 2020
Amended to not revert the changes to be added in PR #5514.
osschar pushed a commit to osschar/root that referenced this pull request Dec 21, 2020
Amended to not revert the changes to be added in PR root-project#5514.
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.

3 participants