[hist] Improve limits of THLimitsFinder.

[hageboeck]

THLimitsFinder sometimes removes the first/last bin of an axis range. Here, it is ensured that the min and max of a buffered range is part of the axis range, and not removed by accident.
This fixes issues with TTree::Draw such as the one described in https://root-forum.cern.ch/t/bug-or-feature-in-ttree-draw/62862/

This supersedes #17689

[github-actions]

Test Results

    21 files      21 suites   3d 12h 18m 41s ⏱️
 3 391 tests  3 301 ✅  0 💤  90 ❌
69 490 runs  68 602 ✅ 29 💤 859 ❌

For more details on these failures, see this check.

Results for commit 99a5fe0.

♻️ This comment has been updated with latest results.

[ferdymercury]

LGTM thanks!

I would propose this test:

TTree t;
Float_t x;
t.Branch("x", &x);
x = -999;
t.Fill();
x = 0;
t.Fill();
t.Draw("x");
auto h = (TH1*)gROOT->FindObject("htemp");
ASSERT_EQ(h->GetEntries(), h->GetEffectiveEntries());

[hageboeck]

LGTM thanks!

I would propose this test:

TTree t;
Float_t x;
t.Branch("x", &x);
x = -999;
t.Fill();
x = 0;
t.Fill();
t.Draw("x");
auto h = (TH1*)gROOT->FindObject("htemp");
ASSERT_EQ(h->GetEntries(), h->GetEffectiveEntries());

@ferdymercury good test! It's added now.

[ferdymercury]

Thanks! Just a nitpick at the end of DrawAutoBinning:

delete h;
delete gROOT->FindObject("c1");

[hageboeck]

Thanks! Just a nitpick at the end of DrawAutoBinning:

delete h;
delete gROOT->FindObject("c1");

👍, updated for h. The canvas isn't created in batch mode, though, so that should be be unnecessary in this test.

[ferdymercury]

The canvas isn't created in batch mode, though, so that should be be unnecessary in this test.

Are you sure?

root -l -b
root [0] TTree t;
root [1] Float_t x;
root [2] t.Branch("x", &x);
root [3] x = -999;
root [4] t.Fill();
root [5] x = 0;
root [6] t.Fill();
root [7] t.Draw("x");
Info in <TCanvas::MakeDefCanvas>:  created default TCanvas with name c1
root [8] gROOT->FindObject("c1")
(TObject *) 0x5c740608b9c0
root [9] delete gROOT->FindObject("c1")
root [10] gROOT->FindObject("c1")
(TObject *) nullptr

[hageboeck]

The test failures for the graphics are real. This will require more investigation.

[ferdymercury]

Shouldn't it be al > BinLow + BinWidth ? instead of < ?
Or maybe even easier:

Suggested change

	if (al - BinLow >= atest && al < BinLow + BinWidth) {
	// Suppress the first bin, but only if al doesn't fall into it
	BinLow += BinWidth;
	nbins--;
	}
	if (BinHigh - ah >= atest && BinHigh - BinWidth > ah) {
	// Suppress the last bin, but only if ah doesn't fall into it
	BinHigh -= BinWidth;
	nbins--;
	}
	if (al >= BinLow + BinWidth + atest) {
	// Suppress the first bin, but only if al doesn't fall into it
	BinLow += BinWidth;
	nbins--;
	}
	if (ah <= BinHigh - BinWidth - atest) {
	// Suppress the last bin, but only if ah doesn't fall into it
	BinHigh -= BinWidth;
	nbins--;
	}

[hageboeck]

Yes, indeed.
But looking at this logic again, it's clear that it may truncate the axis range, so one shouldn't use it. At the same time, I cannot remove it (I tested), because this breaks the "looks" of other graphics components.
I think I now found a solution that works. It's already pushed.