root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.55k stars 1.24k forks source link

TH1D::GetRandom and histograms with non-uniform binning #9530

Open andrea-celentano opened 2 years ago

andrea-celentano commented 2 years ago

Assume to have a TH1D object, with non-uniform binning, representing some kind of differential quantity (for example, a particle flux as a function of the particle energy, dN/dE). The TH1D::GetRandom method will return random numbers distributed according to the histogram bins height, rather than the histogram bins area.

The code attached here (a ROOT macro) reproduces this behavior (I attach it as a .txt file, since .C files are not supported) test.txt

couet commented 2 years ago

It seems think that's not graphics and that more for @lmoneta

andrea-celentano commented 2 years ago

Hi, I am trying to follow the instructions reported here to fork the ROOT repository, create a new branch, suggest a fix, and then do a pull request. I agree that this is not a graphic issue, rather a "numerical" one.

andrea-celentano commented 2 years ago

I made a pull request for this (9538), but I do not know how to link that to this issue (there is a "Linked pull requests" list on the web page, but I cannot modify it).

lmoneta commented 2 years ago

Hi,

I think this is not a bug, but maybe a different way of representing the non-uniform binning histogram. You first scale the original histogram by the bin width to represent a density, this is fine and then you generate random number from this scaled histogram. If you doing this you the histogram represent now a density and of course you need to account of the bin width when callingTH1::GetRandom. If you would use the original un-scaled histogram, it would be fine. I am afraid adding the fix that you suggest it will add some inconsistency. Maybe the way to proceed is adding in GetRandom an option to consider the bin width in case of density histograms ?

Cheers

Lorenzo

andrea-celentano commented 2 years ago

Dear Lorenzo, I agree that this is a matter related to what a histogram is, and what the operation of extracting random numbers from it should be. I am totally in favor of adding an option in TH1::GetRandom to allow both solutions.