[hist] Fix GetRandom with negative bins - #22980
Open
shuv-amp wants to merge 1 commit into
Open
Conversation
The negative-bin check in ComputeIntegral(true) used break, which only left the inner loop. In 2D and 3D, later rows could overwrite the NaN marker. GetRandom, GetRandom2 and GetRandom3 also compared the result directly to NaN, so their error paths were never taken. Return immediately without caching a partial integral, use std::isnan in the GetRandom methods, and make the TH2 and TH3 FillRandom overloads validate the source before its integral is cached. This also keeps TH2Poly::GetRandom2 from calling BinarySearch with a null integral.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ComputeIntegral(true)usedbreakafter finding a negative bin, which only exited the innermost loop. In two and three dimensions, later rows or planes could overwrite the NaN marker.GetRandom,GetRandom2, andGetRandom3also compared the result directly to NaN, so they did not detect the failure.Return immediately without caching the partial integral, check the result with
std::isnan, and make the TH2 and TH3FillRandomoverloads validate the source before caching its integral. This also preventsTH2Poly::GetRandom2from callingBinarySearchwith a null integral.The regression tests cover one-, two-, and three-dimensional histograms, the TH2 and TH3
FillRandompaths, andTH2Poly. Tested locally withtestTH1and the Hist GoogleTest suite.AI disclosure: I used AI while implementing this fix. I reviewed and tested the changes and take responsibility for them.