[hist] do not look at bin center if one edge is infinite #20176
Conversation
Test Results 22 files 22 suites 3d 15h 36m 20s ⏱️ For more details on these failures, see this check. Results for commit 655cdc7. ♻️ This comment has been updated with latest results. |
hist/hist/src/TAxis.cxx
Outdated
| /// If fXmin is -inf, it will return -nan even if fXmax is finite | ||
| /// If fXmax is +inf, it will return +inf even if fXmin is finite | ||
|
|
||
| Double_t TAxis::GetBinCenter(Int_t bin) const | ||
| { | ||
| Double_t binwidth; | ||
| if (!fXbins.fN || bin<1 || bin>fNbins) { | ||
| binwidth = (fXmax - fXmin) / Double_t(fNbins); | ||
| return fXmin + (bin-1) * binwidth + 0.5*binwidth; | ||
| return fXmin + (bin - 0.5) * binwidth; |
There was a problem hiding this comment.
I have some doubts about this logic since we're introducing handling of infinite values. To me it's not so clear that they should not be treated separately. For example, I would have assumed we would have just added some logic to the function of the type
double ret = fXmin + (bin - 0.5) * binwidth;
if (std::isinf(ret) || std::isnan(ret))
return std::numeric_limits<double>::quiet_NaN();
At least to have a homogeneous return value in this class of exceptional cases. I guess this also relates to users' expectations and how they would need to treat this afterwards. Thoughts @lmoneta ?
There was a problem hiding this comment.
Hmmm my view: I'd say that the bin center return value is correctly defined, it's infinite. It's not NaN.
So the problem is that one should not rely on GetBinCenter for finding corresponding bins between two axes.
A more elegant solution might be just to look linearly bin by bin what are the edges of each of the two axes and then create map from one to the other. This way one would not rely at all on GetBinCenter and eps.
But I thought the solution with eps was simple and fast enough.
Of course, another approach would be to say: infinite edges are not allowed in TH1, use instead 1e30 or sth like that, and then we get rid of the complexity of dealing with this.
There was a problem hiding this comment.
I'd say that the bin center return value is correctly defined, it's infinite. It's not NaN.
It's only infinite in case of the positive infinite upper bin edge. In case of the negative infinite lower bin edge, then it's going to be a NaN because of fXmin + (bin - 0.5) * binwidth; becoming practically a -inf + inf computation. So I don't see that the return value it's always correctly defined.
There was a problem hiding this comment.
You're right!
The drawback of all this is that we should update many function to deal with this.
GetBinLowEdge
GetBinUpEdge
GetBinWidth
GetBinCenterLog
So maybe we should just say -inf/+inf is unsupported? (workaround for the user is to just use 1e30 in that case?
Fixes root-project#20174 [hist] specifically dealing with edge cases
ferdymercury
force-pushed
the
infcenter
branch
from
ad21d2f to
655cdc7
Compare
2 participants
This Pull request:
Changes or fixes:
Fixes #20174
Checklist:
This PR fixes #