[TREE] Utility to enter a range of entries in TEntryList by vepadulano · Pull Request #8740 · root-project/root

vepadulano · 2021-07-27T09:50:57Z

Add a utility function to call TEntryList::Enter with entries in a certain range, instead of having to do the loop manually. Especially useful in PyROOT to avoid doing the same in a Python loop

Initial idea

>>> import ROOT
>>> e = ROOT.TEntryList()
>>> e.GetN()
0
>>> ROOT.ROOT.Detail.EnterRange(e, 0, 10)
>>> e.GetN()
10

Not sure about the namespace and the naming, can be discussed

Final interface

>>> import ROOT
>>> e = ROOT.TEntryList()
>>> e.EnterRange(0,10)
>>> e.GetN()
10

phsft-bot · 2021-07-27T09:51:18Z

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

eguiraud

Idea + implementation looks good to me.

Naming/placement: if we are putting the function in TEntryList.h anyway, I guess it can be a method. @pcanal what do you think?

Unrelated, but I hope you don't really need the double ROOT.ROOT to call this from PyROOT, I think that has been fixed years ago 😄

tree/tree/src/TEntryList.cxx

vepadulano · 2021-07-27T10:15:51Z

Unrelated, but I hope you don't really need the double ROOT.ROOT to call this from PyROOT, I think that has been fixed years ago

I thought so too, but currently

>>> ROOT.Detail.EnterRange
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: <namespace cppyy.gbl.Detail at 0x56507072a2f0> has no attribute 'EnterRange'. Full details:
  type object 'Detail' has no attribute 'EnterRange'
  'Detail::EnterRange' is not a known C++ class
  'EnterRange' is not a known C++ template
  'EnterRange' is not a known C++ enum

So maybe it's something that must be "activated" somehow. I'd need to see how it's done with other classes/functions

eguiraud · 2021-07-27T10:27:09Z

It could be that it only works for classes and not for functions. Can you please open an issue about that? It's unrelated to the PR but I can't find a jira ticket about this ROOT.ROOT thing and I'm not sure we are happy with it not working sometimes.

phsft-bot · 2021-07-27T10:51:55Z

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-27T11:10:02Z

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

Failing tests:

projectroot.runtutorials.tutorial_roofit_rf408_RDataFrameToRooFit_py

vepadulano · 2021-07-27T13:30:30Z

Can you please open an issue about that?

See #8745

phsft-bot · 2021-08-09T14:15:09Z

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

eguiraud

Alright, looks good to me!

The feature needs a test before merging.

Also maybe I'd put the tree argument before the step argument because I don't expect the step argument to be used much? (but maybe that's also the case for tree?)

vepadulano · 2021-08-09T14:33:08Z

Also maybe I'd put the tree argument before the step argument because I don't expect the step argument to be used much? (but maybe that's also the case for tree?)

I put the step first because I felt it went "logically" after the other two integer arguments. I really wouldn't know what could be used more between that and the tree argument

phsft-bot · 2021-08-10T07:26:00Z

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-08-10T08:10:21Z

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.tree.tree.test.gtest_tree_tree_test_entrylist_enterrange

phsft-bot · 2021-08-10T08:28:50Z

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.tree.tree.test.gtest_tree_tree_test_entrylist_enterrange

phsft-bot · 2021-08-10T08:44:15Z

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

Failing tests:

projectroot.tree.tree.test.gtest_tree_tree_test_entrylist_enterrange

vepadulano · 2021-08-10T09:04:35Z

The test failures report the file created during the test does not exist, but this node ran the test fine https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/124359/testReport/projectroot.tree.tree/test/gtest_tree_tree_test_entrylist_enterrange/

Probably using the same filename for multiple tests leads to a recreation/deletion of the file that is then not usable by the new test?

phsft-bot · 2021-08-10T09:14:48Z

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-08-10T09:53:35Z

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

Errors:

[2021-08-10T09:15:42.821Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory

phsft-bot · 2021-08-10T12:50:52Z

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

Failing tests:

eguiraud · 2021-08-10T13:02:37Z

Using the same file in multiple tests is never good, they might run concurrently

vepadulano · 2021-08-10T13:10:51Z

Using the same file in multiple tests is never good, they might run concurrently

You are right, I changed the filename in the latest commit 👍

eguiraud

LGTM

tree/tree/test/entrylist_enterrange.cxx

A new method `EnterRange` has been added to TEntryList to allow adding all entries in a certain range in a loop, rather than having to do the loop explicitly in the user application. This is especially helpful in PyROOT to avoid having to do the loop in Python.

phsft-bot · 2021-08-10T13:35:41Z

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

vepadulano · 2021-08-10T13:35:57Z

Added commit message and rebased. Will merge if tests pass

vepadulano added the in:TTree label Jul 27, 2021

vepadulano requested review from eguiraud and pcanal July 27, 2021 09:50

vepadulano self-assigned this Jul 27, 2021

eguiraud approved these changes Jul 27, 2021

View reviewed changes

tree/tree/src/TEntryList.cxx Outdated Show resolved Hide resolved

eguiraud reviewed Aug 9, 2021

View reviewed changes

vepadulano mentioned this pull request Aug 9, 2021

[DF] TEntryList instead of Range for TTree based distributed RDF #8646

Merged

3 tasks

vepadulano requested a review from bellenot as a code owner August 10, 2021 07:25

eguiraud approved these changes Aug 10, 2021

View reviewed changes

tree/tree/test/entrylist_enterrange.cxx Outdated Show resolved Hide resolved

vepadulano added 2 commits August 10, 2021 15:33

[TEntryList] Add test for TEntryList::EnterRange

9839af2

vepadulano force-pushed the tentrylist-enterrange branch from 30b7f94 to 9839af2 Compare August 10, 2021 13:35

vepadulano merged commit 293a52b into root-project:master Aug 10, 2021

vepadulano deleted the tentrylist-enterrange branch October 25, 2021 15:24

Conversation

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

Uh oh!

Initial idea

Final interface

Uh oh!

phsft-bot commented Jul 27, 2021

Uh oh!

eguiraud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vepadulano commented Jul 27, 2021

Uh oh!

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

Uh oh!

Uh oh!

phsft-bot commented Jul 27, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 27, 2021

Failing tests:

Uh oh!

vepadulano commented Jul 27, 2021

Uh oh!

phsft-bot commented Aug 9, 2021

Uh oh!

eguiraud left a comment

Choose a reason for hiding this comment

Uh oh!

vepadulano commented Aug 9, 2021

Uh oh!

phsft-bot commented Aug 10, 2021

Uh oh!

phsft-bot commented Aug 10, 2021

Failing tests:

Uh oh!

phsft-bot commented Aug 10, 2021

Failing tests:

Uh oh!

phsft-bot commented Aug 10, 2021

Failing tests:

Uh oh!

vepadulano commented Aug 10, 2021

Uh oh!

phsft-bot commented Aug 10, 2021

Uh oh!

phsft-bot commented Aug 10, 2021

Errors:

Uh oh!

phsft-bot commented Aug 10, 2021

Failing tests:

Uh oh!

eguiraud commented Aug 10, 2021

Uh oh!

vepadulano commented Aug 10, 2021

Uh oh!

eguiraud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phsft-bot commented Aug 10, 2021

Uh oh!

vepadulano commented Aug 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vepadulano commented Jul 27, 2021 •

edited

Loading

eguiraud commented Jul 27, 2021 •

edited

Loading