TH1 KolmogorovTest infinite loop #705
Conversation
When two histograms (A, B) are tested using the "X" method a new histogram (C) is created which is then subsequently filled using random data based off of A. If A contains bins with negative content an infinite loop can occur because of internals of TH1::GetRandom. This was discovered using simulated physics data where neg. weights do occur and a fine binning of a distribution histogram. As long the negatives are small we should be able to ignore them, however this can in change the result and so a warning is printed.
When the hist contains negative content, `h->GetRandom()` will return NaN. (Unless the integral is first computed by `h->ComputeIntegral(false)` and subsequently cached...). This makes `fXaxis.FindBin(x=NaN)` return the right-most bin. If this bin has content 0 an infinite loop will ensue. This patch makes it an error to call FillRandom with a histgram containing negative bin content.
|
Can one of the admins verify this patch? |
|
@phsft-bot build and stop spamming |
|
Starting build on |
|
Build failed on centos7/gcc49. Errors:
Warnings:
|
|
Build failed on mac1012/native. Warnings:
Failing tests: |
|
Hi, including the patch in my build I have failed to reproduce the problem on the same machine where it occurred (mac1012/native). |
|
Sounds good, thanks for looking into it! Should I extend the reproducer into a full test case? |
|
Hi, who is the TH1 developer? I think it is time to check in with him :) |
ashlaban
force-pushed
the
20170628-th1-kolmogorov-inf-loop
branch
from
0982be1 to
fc02c98
Compare
|
Ping @lmoneta. Updated according to our discussion. |
5 participants
[EDIT]: 2017-07-20 dropped commit (6953483).
See the commit messages for detailed descriptions of the changes. In essence, when comparing some data with the KolmogorovTest an infinite loop was triggered.
(bba95dd) We want to get an approximate solution to the test despite there being some bins with neg. content
(06024c8) To prevent the infinite loop in other cases, calling FillRandom with a histogram with negative content is now an error.
(6953483) The caching inside GetRandom can violate an invariant of the function. (Always return NaN when there is a bin with neg. content in the source histogram). [EDIT] Removed after discussion with Lorenzo. The recomputation of the integral here was deemed too expensive as the function is intended to be called in tight loops.
Since these changes might have far reaching effects I am up for discussion whether any commits should be dropped.
A reproducer in can be found here.