Skip to content

Conversation

@aapo-kossi
Copy link
Contributor

@aapo-kossi aapo-kossi commented Oct 28, 2025

Since outgoing bin indices can be computed depending only on the option to keep original binning and set axis ranges, there is no need to use FindBin and GetBinCenter to get the output bin where integrated contents are to be placed. This PR removes the usage of GetBinCenter and FindBin from the output bin index calculation, instead using simple index arithmetic. This fixes the corner-case where non-finite bin edges cause projected bins to be empty due to GetBinCenter returning a bin index that is not actually inside the bin. A slight performance improvement could also be expected, but this is unimportant since I don't expect a histogram projection to be a bottleneck in anyone's analysis.

I tried adding more tests to verify that the new logic is sound and equivalent to the existing implementation in cases other than the mentioned infinite bin edges. I think someone more familiar with the internals should make sure I have covered all use cases so that this change doesn't introduce any new bugs.

Fixes #20174

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #20174

Since outgoing bin indices can be computed depending only on
the option to keep original binning and set axis ranges, there
is no need to use FindBin and GetBinCenter to get the output bin
where integrated contents are to be placed. This PR removes the usage
of GetBinCenter and FindBin from the output bin index calculation,
instead using simple index arithmetic. This fixes the corner-case where
non-finite bin edges cause projected bins to be empty due to
GetBinCenter returning a bin index that is not actually inside the bin.
A slight performance improvement could also be expected, but this is
unimportant since I don't expect a histogram projection to be a bottleneck
in anyone's analysis.

I tried adding more tests to verify that the new logic is sound and
equivalent to the existing implementation in cases other than the mentioned
infinite bin edges. I think someone more familiar with the internals should
make sure I have covered all use cases so that this change doesn't
introduce any new bugs.

Fixes root-project#20174
@aapo-kossi aapo-kossi requested a review from lmoneta as a code owner October 28, 2025 13:38
@aapo-kossi
Copy link
Contributor Author

aapo-kossi commented Oct 28, 2025

This should be viewed as an alternative approach to #20176, also related to #20201. If/while infinite bin edges are fully disallowed then this is mostly irrelevant and probably unnecessary, but I do not know if that is needed if a fix is this simple.

@ferdymercury
Copy link
Collaborator

Thanks! Side note: the drawback of supporting this is that one would have also have to fix GetBinCenter, GetBinLowEdge independently of whether it's used or not for the projection), and fixing it might affect performance.
Likewise, the definition of the overflow and underflow bin lose meaning when the first bin is already -inf

@github-actions
Copy link

Test Results

    22 files      22 suites   3d 17h 56m 24s ⏱️
 3 701 tests  3 699 ✅ 0 💤 2 ❌
79 489 runs  79 487 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit a17cc15.

@aapo-kossi
Copy link
Contributor Author

Thanks! Side note: the drawback of supporting this is that one would have also have to fix GetBinCenter, GetBinLowEdge independently of whether it's used or not for the projection), and fixing it might affect performance. Likewise, the definition of the overflow and underflow bin lose meaning when the first bin is already -inf

I agree with the sentiment that those methods could ideally return something reasonable, but since the returned values will either be infinite or NaN, they shouldn't be relied upon for any calculation other than possibly checking that they are not finite. The current implementation seemingly can already do this, though with some inconsistency on if the return value is inf or NaN.

To be more concrete, I don't expect a bin center especially to be very meaningful if we are talking about a bin with infinite width. Having a finite width for the bin in question could just be a precondition for using that method in my opinion, for example. And for getting the bin edges, I don't think uniformly binned axes should need to accept infinities.

A flow bin mostly does become meaningless for a histogram extending to infinity, yes (except for filling at +inf itself).

All this to say that there may be other problems with accepting infs that may need to be considered together as a whole, but in isolation this PR doesn't actually do any handling of infinities. It does fix an issue that would prevent them from being supported and in my opinion it simplifies the projection operations by removing unnecessary checking of axis coordinates (given I haven't missed a case that actually makes it necessary).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TH1 and derivates projections produce incorrect results for histograms with infinite bin edges

3 participants