[hist] Implement RBinIndexMultiRange#21322
[hist] Implement RBinIndexMultiRange#21322hahnjo wants to merge 6 commits intoroot-project:masterfrom
RBinIndexMultiRange#21322Conversation
hahnjo
force-pushed
the
hist-multi-range
branch
from
aec4ccb to
53eb18b
Compare
Test Results 22 files 22 suites 3d 11h 30m 0s ⏱️ For more details on these failures, see this check. Results for commit dc110c9. ♻️ This comment has been updated with latest results. |
| for (; j < N; j++) { | ||
| // Reverse iteration order to advance the innermost index first. | ||
| const std::size_t i = N - 1 - j; |
There was a problem hiding this comment.
One could consider:
for (; i > 0; i--) {This might be easier to read.
There was a problem hiding this comment.
This doesn't work unfortunately since the indices are (unsigned) std::size_t which is always >= 0 and wraps around...
| for (auto &&range : fRanges) { | ||
| it.fIterators.push_back(range.end()); | ||
| // NB: We dereference and this is fine because we know the implementation of RBinIndexRange, in the worst case | ||
| // the returned RBinIndex will be invalid. | ||
| it.fIndices.push_back(*it.fIterators.back()); | ||
| } |
There was a problem hiding this comment.
This looks expensive to construct. This may be fine, though, if it's used in range-based for loops.
It would not be fine in:
for (. ; it < multiRange.end(); ++it) {
I don't know if that's supposed to be supported?
There was a problem hiding this comment.
Right, I didn't consider this pattern. I noticed that RBinIndexMultiRange::RIterator is "expensive" to copy, but I concluded that not much can be done about it. For end() though we can "cheat" and always return the empty iterator, which should be ~cheap to construct without dynamic memory allocations. Dereferencing it doesn't return the "expected" result anymore (an empty fIndices), but that's not allowed by the C++ standard anyway.
| TEST(RBinIndexMultiRange, Multi) | ||
| { | ||
| const auto index0 = RBinIndex(0); | ||
| const auto normal = CreateBinIndexRange(index0, RBinIndex(10), 0); |
There was a problem hiding this comment.
It's internal, but it seems to go against the documentation, which states:
/// \param[in] begin the begin of the bin index range (inclusive)
/// \param[in] end the end of the bin index range (exclusive)
/// \param[in] nNormalBins the number of normal bins, after which iteration advances to RBinIndex::Overflow()
There was a problem hiding this comment.
I assume you are referring to nNormalBins = 0? For ranges from a normal begin to normal end, we never advance to the overflow bin, so it is ok to pass in 0. We do the same in RRegularAxis::GetNormalRange(), for example.
It allows convenient iteration over multiple RBinIndexRange, which will be used to implement future operations on histograms, such as projections, rebinning, slicing.
The implementation can be reused for a std::vector of RBinIndex.
Test iteration and summation over all bins.
hahnjo
force-pushed
the
hist-multi-range
branch
from
53eb18b to
dc110c9
Compare
2 participants
It allows convenient iteration over multiple
RBinIndexRange, which will be used to implement future operations on histograms, such as projections, rebinning, slicing.