[df] First integration of RHist filling
#20664
Conversation
As the classes are experimental, using the method will print a warning to inform the user.
This needs one indirection to construct the std::tuple and separate the weight argument.
| /// ### Example usage: | ||
| /// ~~~{.cpp} | ||
| /// auto h = std::make_shared<ROOT::Experimental::RHist<double>>(10, {5.0, 15.0}); | ||
| /// auto myHist = myDf.Hist(h, {"col0"}); | ||
| /// ~~~ | ||
| template <typename ColumnType = RDFDetail::RInferredType, typename... ColumnTypes, typename BinContentType> | ||
| RResultPtr<ROOT::Experimental::RHist<BinContentType>> | ||
| Hist(std::shared_ptr<ROOT::Experimental::RHist<BinContentType>> h, const ColumnNames_t &columnList) |
There was a problem hiding this comment.
I believe we should market this overload as "bring your own histogram" 😅
Jokes aside, this will enable some nice use cases, for example filling a shared histogram from multiple computation graphs, as demonstrated by the RDFHist.PtrRunGraphs unit test.
Test Results 21 files 21 suites 3d 18h 43m 15s ⏱️ Results for commit 46f531b. |
hahnjo
left a comment
hahnjo
left a comment
There was a problem hiding this comment.
Things to discuss during review (after the break)
| { | ||
| std::shared_ptr h = std::make_shared<ROOT::Experimental::RHist<BinContentType>>(std::move(axes)); | ||
| if (h->GetNDimensions() != columnList.size()) { | ||
| throw std::runtime_error("Wrong number of columns for the specified number of histogram axes."); |
There was a problem hiding this comment.
In histv7, we are consistently throwing std::invalid_argument while RDF prefers std::runtime_error. I think we have to decide which "consistency" to follow...
| /// ROOT::Experimental::RRegularAxis axis(10, {5.0, 15.0}); | ||
| /// auto myHist = myDf.Hist({axis}, {"col0"}); | ||
| /// ~~~ | ||
| template <typename BinContentType = double, typename ColumnType = RDFDetail::RInferredType, typename... ColumnTypes> |
There was a problem hiding this comment.
One could also argue for BinContentType = int or long as default since it's unweighted filling. However, then a number of "post-processing steps" require conversion, for example scaling...
| /// A list containing the names of the columns that will be passed when calling `Fill`. | ||
| /// (N columns for unweighted filling, or N+1 columns for weighted filling) |
There was a problem hiding this comment.
Reminder to ourselves that we want to change this, and backport to 6.38 a warning message
| struct HistoND{}; | ||
| struct HistoNSparseD{}; | ||
| struct Hist{}; | ||
| struct HistWithWeight{}; |
There was a problem hiding this comment.
Having a second tag felt like the easiest solution, together with just a template argument bool WithWeight = false for RHistFillHelper. Other approaches are certainly possible...
1 participant
As the classes are experimental, using the method will print a warning to inform the user.
Note that this is only a first version, there are a couple of advanced functionalities and better error handling that will come in future PRs.