Skip to content

[skip-ci] clarify behavior and rounding precision limitations of FindBin #17899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Mar 6, 2025

This Pull request:

Changes or fixes:

See https://root-forum.cern.ch/t/bug-or-feature-in-ttree-draw/62862/16

Related: #17689

@ferdymercury ferdymercury requested a review from lmoneta as a code owner March 6, 2025 08:39
@guitargeek
Copy link
Contributor

Does this maybe interfere with #14105, where I have made some suggestions to improve the numerical stability of FindBin()? That PR is kind of stuck, because then we would also have to improve other functions like GetBinLowEdge and GetBinHighEdge to be consistent. And changing the numeric behavior is always a risk for backwards compatibility, even if it goes in the right direction :|

We need to check whether the example TAxis(1, -1., 0.).FindBin(-1e-17) still wrongly returns the overflow bin even with the other PR merged. Thank we can merge this PR without having to worry that it gets invalidated by #14105.

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Mar 6, 2025

Thanks for the feedback!

We need to check whether the example TAxis(1, -1., 0.).FindBin(-1e-17) still wrongly returns the overflow bin even with the other PR merged.

I checked with that PR you have mentioned, and it still returns the wrong bin.
So it does not really interfere with the other pull request. This PR would be just documenting the current (and potentially future) behaviour.

See reproducer with the other PR:

      double x = -1e-17;
      const double fXmin = -1.;
      const double fXmax = 0.;
      const int fNbins = 1;
      // shift x and fXmax by fXmin:
      x = x - fXmin;
      const double b = fXmax - fXmin;
      const double s = fNbins * x; // scaled version of x
      double m = std::floor(s / b);
      // iterative correction in case of floating point errors due to floating point division
      while (m * b > s) // if left bin boundary is greater then x, decrement
         m = m - 1;
      while ((m + 1) * b <= s) //if right bin boundary is smaller or equal, increment
         m = m + 1;
      cout << 1 + Int_t(m) << endl;

If we cast everything to long-double, then the same issue appears a bit later, at -1e-20, both in the current master implementation as well as in the referenced PR.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR and following up to my comment! The PR looks good to me.

@guitargeek guitargeek merged commit 1703c54 into root-project:master Mar 7, 2025
5 checks passed
@ferdymercury ferdymercury deleted the findbinprec branch March 7, 2025 10:38
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.

2 participants