[histv7] Division for RHist.#5497
Conversation
…ts for Add in RHist.
…agated yet, but the structure to do so is implemented.
|
Starting build on |
|
Build failed on ROOT-fedora30/cxx14. Failing tests: |
|
Build failed on mac1015/cxx17. Failing tests:
|
Axel-Naumann
left a comment
Axel-Naumann
left a comment
There was a problem hiding this comment.
Super nice, congrats! Some tiny suggestions.
|
|
||
| /// Divide by an other RHistStatTotalSumOfWeights data, assuming same bin configuration. | ||
| void DivideBinomial(const RHistStatTotalSumOfWeights& other) { | ||
| // /!\ Should be the sum of the updated weights (after division) of the bin contents |
There was a problem hiding this comment.
Out of curiosity: what's /!\ Looks great for a warning sign, does Doxygen understand that? And maybe:
| // /!\ Should be the sum of the updated weights (after division) of the bin contents | |
| // /!\ Must be called *after* the bin contents have been divided! |
(if that's what it is). More occurrences below.
HadrienG2
left a comment
HadrienG2
left a comment
There was a problem hiding this comment.
Looks reasonable to me at this stage, but there are a couple of things which I don't understand...
|
Starting build on |
|
Build failed on ROOT-fedora29/python3. Failing tests: |
| /// Adding histograms with incompatible axis binning will be reported at runtime | ||
| /// with an `std::runtime_error`. Insufficient statistics in the source | ||
| /// histogram will be detected at compile-time and result in a compiler error. | ||
| /// with an `std::runtime_error`. Incompatible statistics' size in the source |
There was a problem hiding this comment.
It's not just about size, the actual constraint (as of the current code) is that "from" must record at least the same statistics as "to" or else the toImpl.GetStat().Add(fromImpl.GetStat()) call will fail because the statistics which only exist in "to" will not find their counterpart in "from" and therefore the call to the corresponding virtual statistics class method in the implementation will fail.
|
Closing this PR for the moment due to #18191 |
6 participants
No description provided.