Skip to content

[hist] in TTree::Draw, last bin should include vmax values #17689

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

Closed
wants to merge 2 commits into from

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Feb 10, 2025

This Pull request:

Changes or fixes:

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

Rounding issue reproducer::

TH1F h("h","h", 100, -1000, 1e31*std::nextafter(float(0),INFINITY)); h.Fill(-999); h.Fill(0); h.GetBinContent(100)

changing to 1e32 makes it work.

It comes down to this line in TAxis:

bin = 1 + int (fNbins*(x-fXmin)/(fXmax-fXmin) );

so:

int(100*1000./(1000.+1e-13)) is 99
whereas
int(100*1000./(1000.+1e-14)) is 100

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

Copy link

github-actions bot commented Feb 11, 2025

Test Results

    18 files      18 suites   4d 21h 46m 10s ⏱️
 2 715 tests  2 715 ✅ 0 💤 0 ❌
47 172 runs  47 172 ✅ 0 💤 0 ❌

Results for commit 0d57683.

♻️ This comment has been updated with latest results.

@ferdymercury
Copy link
Collaborator Author

I would propose merging this soonish, since it's silently hiding data. (Hiding them in the overflow)

@hageboeck
Copy link
Member

I would propose merging this soonish, since it's silently hiding data. (Hiding them in the overflow)

Hello @ferdymercury,

Thanks for your persistence on this issue! 🙂

I agree that it's an annoying "feature", so it needs to be fixed. I looked again today, and believe I found the underlying cause:
OptimizeLimits removes the first/last bin from the range based on a slightly arbitrary distance criterion.
By adding a check that min and max are within the axis range, I managed to fix the issue. You can see the relevant change in #19506. There is a good argument to doing it in that function, because the check I added uses the same floating-point arithmetic as the computation of the axis range, so it's guaranteed that min/max fall within the valid range (unless I didn't get < vs <= right, please have a look!).
#19605 is in draft for the moment, because I plan to add a test also.

@ferdymercury
Copy link
Collaborator Author

Thanks Stephan for looking into it and proposing an alternative PR!

@ferdymercury ferdymercury deleted the xmaxlim branch August 11, 2025 14:46
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.

4 participants