Add N-dimensional and more flexible histogram filling to RDataFrame by bendavid · Pull Request #7499 · root-project/root

bendavid · 2021-03-14T18:27:30Z

This PR does a few things

Extends the RDF Fill functionality to support arbitrary types and number of columns and an arbitrary mix of individual objects and containers with variadic templates.

Note that this will likely result in slower code being generated in case of compiling/jitting without optimization.

Adds a HistoND function to RDF to fill a THnD with arbitrary number of dimensions

The main issue here was actually that THnT does not have a publicly accessible copy constructor or assignment operator, which are needed for use with RDF. I didn't have the patience to implement this by hand for all the classes in the inheritance chain, so the relevant classes have been migrated from C-style arrays to std::vector such that default copy (and move) constructors and assignment operators can be automatically generated.

Appropriate constructors have been added to allow THnT to be used with variable binning.

phsft-bot · 2021-03-14T18:27:32Z

Can one of the admins verify this patch?

eguiraud · 2021-03-15T08:14:34Z

Hi @bendavid , thank you for the contribution, I will take a look as soon as possible

eguiraud · 2021-03-18T14:34:21Z

We need @lmoneta to review the changes in THn and TNDArray

eguiraud · 2021-03-18T14:40:31Z

@bendavid about a), a move to C++14 for ROOT6 is foreseen but the timescale is too long to matter for this PR, I'm afraid.

lmoneta

Looks good to me. I leave to @Axel-Naumann author of the THn classes to give the final approval.

eguiraud

This is great!

addition of a copy-constructor in THn: good (a move-constructor should also be autogenerated right?)
generalization of the different FillParHelper::Exec overloads into a single implementation: good, I actually find it pretty readable considering the inherent complexity of what it is doing. No fold expressions and constexpr if unfortunately, not much we can do about that
addition of a FillN method to RDF: great, certainly useful and it even reuses a lot of the existing infrastructure

As mentioned above, we need @Axel-Naumann to review the THn-related changes. However, I don't know if the "temporary hack" to allow copy-construction of THn's is really temporary, what are the drawbacks?

See below a bunch of minor comments.

Only strictly required change: we need a test for FillN (jitted, non-jitted, single-thread, multi-thread): it should be easy to duplicate and adapt one of the tests we have for Fill.

tree/dataframe/inc/ROOT/RDF/Utils.hxx

hist/hist/inc/THn.h

hist/hist/inc/THnBase.h

tree/dataframe/inc/ROOT/RDF/RInterface.hxx

eguiraud · 2021-03-24T14:44:56Z

(will start a PR build after merge conflicts are resolved)

bendavid · 2021-03-24T15:00:04Z

The main drawback of the "temporary hack" is that there is some duplication of information and operations now between the size of the vector and separate integer data members in the class which were previously needed to keep track of the size of the arrays. If migrating from bare arrays to std::vector to allow the compiler to autogenerate the constructors is the preferred solution (rather than implementing them by hand) then I can of course clean this up.

For what concerns the readability of the code in C++11, one thing which could help a little bit and might be feasible is if std::apply were added to the STL backports.

Another concern I had is that this implementation might be a bit slower than the previous individual cases if compiled without optimization (e.g. when jitted).

oshadura · 2021-03-29T17:43:22Z

@bendavid whenever you will have time can you please rebase on master? I will retrigger CI after...

eguiraud · 2021-04-27T15:54:52Z

@Axel-Naumann is it ok to switch THn to std::vectors instead of C-style arrays as it's done in this PR? We get compiler-defined constructors out of it.

@bendavid can you please rebase on master to fix the conflicts when you have time?

bendavid · 2021-04-28T17:25:59Z

Yes I would like some feedback on the use of std::vector so I can take care of that appropriately together with rebasing. If this strategy is agreed I can also clean up a bit more to avoid the redundant size counters, etc.

hist/hist/src/THn.cxx

hist/hist/inc/TNDArray.h

hist/hist/src/THnBase.cxx

Axel-Naumann

The THn part LGTM! A couple of minor comments, but I'm happy with the change from C-style array to vector.

⚠️ NOTE that this change means we need to increase the ClassDef version for all classes with changes base or data members! This should happen before this PR gets merged. Assuming that: ✔️

bendavid · 2021-11-11T00:53:27Z

Alright I've rebased and hopefully addressed all of the comments.

bendavid · 2021-11-11T01:53:04Z

There's something weird going on with clang-format here, where running locally seems to give a different result from the online checks for e.g.

void Exec(unsigned int slot, const Xs &...xs)

vs

void Exec(unsigned int slot, const Xs &... xs)

eguiraud · 2021-11-11T07:59:53Z

Thanks Josh, I'll do one last review pass as soon as I have spare cycles. Re: clang-format, probably different versions disagree on some edge cases, you can ignore the CI complaints.

eguiraud · 2021-11-11T08:00:06Z

@phsft-bot build

phsft-bot · 2021-11-11T08:00:15Z

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

phsft-bot · 2021-11-11T08:34:49Z

Build failed on mac11.0/cxx17.
See console output.

bendavid · 2021-11-20T19:25:46Z

Small update to this to make the new Fill method for THnBase also work with integers (ie allowing the normal set of implicit type conversions, rather than excluding narrowing ones).

eguiraud · 2021-12-15T15:43:40Z

@phsft-bot build

phsft-bot · 2021-12-15T15:43:46Z

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-15T15:45:12Z

Build failed on ROOT-ubuntu2004/soversion.
See console output.

eguiraud

Green for me if green for jenkins, thank you very much @bendavid , this is great

phsft-bot · 2021-12-15T16:18:32Z

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

Failing tests:

projectroot.roottest.root.dataframe.roottest_root_dataframe_typeguessing

phsft-bot · 2021-12-15T16:19:01Z

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-15T16:50:42Z

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

Warnings:

[2021-12-15T15:56:58.236Z] /home/sftnight/build/workspace/root-pullrequests-build/root/hist/hist/src/THnBase.cxx:71:24: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘std::vector<std::vector<double> >::size_type’ {aka ‘unsigned int’} [-Wformat=]
[2021-12-15T15:56:58.236Z] /home/sftnight/build/workspace/root-pullrequests-build/root/hist/hist/src/THnBase.cxx:76:27: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘std::vector<double>::size_type’ {aka ‘unsigned int’} [-Wformat=]

phsft-bot · 2021-12-15T17:01:31Z

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

Failing tests:

projectroot.roottest.root.dataframe.roottest_root_dataframe_typeguessing

phsft-bot · 2021-12-15T17:33:30Z

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

Failing tests:

projectroot.roottest.root.dataframe.roottest_root_dataframe_typeguessing

phsft-bot · 2021-12-15T17:59:03Z

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

Failing tests:

eguiraud · 2021-12-15T23:13:25Z

Uuuh is Jenkins not picking up the latest roottest? Will take a look tomorrow afternoon.

eguiraud · 2021-12-16T09:04:52Z

@phsft-bot build

phsft-bot · 2021-12-16T09:04:58Z

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

eguiraud · 2021-12-16T09:17:34Z

@Axel-Naumann any idea why jenkins keeps checking out an older commit for roottest? https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/132494/consoleFull

Axel-Naumann · 2021-12-16T09:32:48Z

@phsft-bot build
Should be fixed (caused by server migration)

phsft-bot · 2021-12-16T09:32:53Z

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-16T10:51:50Z

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

Warnings:

[2021-12-16T09:45:30.650Z] /home/sftnight/build/workspace/root-pullrequests-build/root/hist/hist/src/THnBase.cxx:71:24: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘std::vector<std::vector<double> >::size_type’ {aka ‘unsigned int’} [-Wformat=]
[2021-12-16T09:45:30.650Z] /home/sftnight/build/workspace/root-pullrequests-build/root/hist/hist/src/THnBase.cxx:76:27: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘std::vector<double>::size_type’ {aka ‘unsigned int’} [-Wformat=]

phsft-bot · 2021-12-16T11:35:34Z

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

Failing tests:

projectroot.tmva.tmva.test.envelope.gtest_tmva_tmva_test_envelope_TMVA_Classification

phsft-bot · 2021-12-16T12:37:11Z

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

Failing tests:

projectroot.tree.dataframe.test.gtest_tree_dataframe_test_dataframe_merge_results

eguiraud · 2021-12-16T13:47:51Z

@bendavid the remaining failures are real (ignoring tmva), can you please take a look?

bendavid · 2021-12-16T15:06:32Z

Should be fixed now. The ND histogram test was using a huge number of bins by mistake.

eguiraud · 2021-12-16T15:12:11Z

@phsft-bot build

phsft-bot · 2021-12-16T15:12:17Z

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-16T15:19:40Z

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

Errors:

[2021-12-16T15:19:07.801Z] C:\build\workspace\root-pullrequests-build\root\core\imt\src\TTaskGroup.cxx(18,10): fatal error C1083: Cannot open include file: 'tbb/task_group.h': No such file or directory [C:\build\workspace\root-pullrequests-build\build\core\imt\Imt.vcxproj]
[2021-12-16T15:19:08.392Z] C:\build\workspace\root-pullrequests-build\root\core\imt\src\ROpaqueTaskArena.hxx(1,10): fatal error C1083: Cannot open include file: 'tbb/task_arena.h': No such file or directory [C:\build\workspace\root-pullrequests-build\build\core\imt\Imt.vcxproj]
[2021-12-16T15:19:08.392Z] C:\build\workspace\root-pullrequests-build\root\core\imt\src\ROpaqueTaskArena.hxx(1,10): fatal error C1083: Cannot open include file: 'tbb/task_arena.h': No such file or directory [C:\build\workspace\root-pullrequests-build\build\core\imt\Imt.vcxproj]

eguiraud · 2021-12-16T17:08:56Z

Windows failure is unrelated, merging.

bendavid requested review from eguiraud and lmoneta as code owners March 14, 2021 18:27

eguiraud self-assigned this Mar 15, 2021

lmoneta reviewed Mar 23, 2021

View reviewed changes

eguiraud suggested changes Mar 24, 2021

View reviewed changes

Axel-Naumann reviewed Apr 29, 2021

View reviewed changes

hist/hist/src/THn.cxx Show resolved Hide resolved

Axel-Naumann reviewed Apr 29, 2021

View reviewed changes

hist/hist/inc/TNDArray.h Outdated Show resolved Hide resolved

Axel-Naumann reviewed Apr 29, 2021

View reviewed changes

hist/hist/src/THnBase.cxx Outdated Show resolved Hide resolved

Axel-Naumann approved these changes Apr 29, 2021

View reviewed changes

bendavid force-pushed the rdf_ndhist branch from f35ee67 to c1bd036 Compare November 11, 2021 00:44

bendavid changed the title ~~[RFC] Add N-dimensional and more flexible histogram filling to RDataFrame~~ Add N-dimensional and more flexible histogram filling to RDataFrame Nov 11, 2021

bendavid force-pushed the rdf_ndhist branch 3 times, most recently from 9af8629 to b07914a Compare November 11, 2021 01:40

eguiraud approved these changes Dec 15, 2021

View reviewed changes

bendavid added 2 commits December 16, 2021 15:00

make format string platform-independent

8d6e30b

reduce number of bins in ND histogram test

446fb11

eguiraud merged commit 5f26326 into root-project:master Dec 16, 2021

bendavid mentioned this pull request Jan 7, 2022

fix copy/move constructors/assignment for THnBase #9512

Merged

hahnjo mentioned this pull request Feb 5, 2026

Improve RInterface::HistoND signature with an extra argument for weight #20816

Closed

Conversation

bendavid commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Mar 14, 2021

Uh oh!

eguiraud commented Mar 15, 2021

Uh oh!

eguiraud commented Mar 18, 2021

Uh oh!

eguiraud commented Mar 18, 2021

Uh oh!

lmoneta left a comment

Choose a reason for hiding this comment

Uh oh!

eguiraud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eguiraud commented Mar 24, 2021

Uh oh!

bendavid commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oshadura commented Mar 29, 2021

Uh oh!

eguiraud commented Apr 27, 2021

Uh oh!

bendavid commented Apr 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Axel-Naumann left a comment

Choose a reason for hiding this comment

Uh oh!

bendavid commented Nov 11, 2021

Uh oh!

bendavid commented Nov 11, 2021

Uh oh!

eguiraud commented Nov 11, 2021

Uh oh!

eguiraud commented Nov 11, 2021

Uh oh!

phsft-bot commented Nov 11, 2021

Uh oh!

phsft-bot commented Nov 11, 2021

Uh oh!

bendavid commented Nov 20, 2021

Uh oh!

eguiraud commented Dec 15, 2021

Uh oh!

phsft-bot commented Dec 15, 2021

Uh oh!

phsft-bot commented Dec 15, 2021

Uh oh!

eguiraud left a comment

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Dec 15, 2021

Failing tests:

Uh oh!

phsft-bot commented Dec 15, 2021

Failing tests:

Uh oh!

phsft-bot commented Dec 15, 2021

Warnings:

Uh oh!

phsft-bot commented Dec 15, 2021

Failing tests:

Uh oh!

bendavid commented Mar 14, 2021 •

edited

Loading

bendavid commented Mar 24, 2021 •

edited

Loading

eguiraud commented Dec 15, 2021 •

edited

Loading