From f97841b1a927bdbc6521628c73de1d09211d5f96 Mon Sep 17 00:00:00 2001 From: Muntasir Mamun Date: Sun, 25 Oct 2026 13:25:42 +0600 Subject: [PATCH 1/4] [graph] Do not abort scatter painting loop on non-positive log coordinates (fixes #23833) - In TGraphPainter::PaintScatter and PaintScatter2D, replace 'else break;' with 'else continue;' in the log scale coordinate checks. - Ensures that non-positive coordinates (x, y, or z <= 0) on logarithmic axes are skipped individually without aborting the rendering of all subsequent valid positive markers. - Add regression unit tests in hist/hist/test/test_TScatter_Log.cxx. --- hist/hist/test/CMakeLists.txt | 3 ++ hist/hist/test/test_TScatter_Log.cxx | 51 ++++++++++++++++++++++++++ hist/histpainter/src/TGraphPainter.cxx | 10 ++--- 3 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 hist/hist/test/test_TScatter_Log.cxx diff --git a/hist/hist/test/CMakeLists.txt b/hist/hist/test/CMakeLists.txt index 7fa26b0a8f1ba..030f73d4ef353 100644 --- a/hist/hist/test/CMakeLists.txt +++ b/hist/hist/test/CMakeLists.txt @@ -60,3 +60,6 @@ ROOT_ADD_TEST(test-tdirectoryfile_destructor_segfault COMMAND tdirectoryfile_des ROOT_ADD_GTEST(test_projections test_projections.cxx LIBRARIES Hist) ROOT_ADD_GTEST(test_th1d_tpad_buildlegend test_th1d_tpad_buildlegend.cxx LIBRARIES Hist Gpad) + +# Test for https://github.com/root-project/root/issues/23833 +ROOT_ADD_GTEST(test_TScatter_Log test_TScatter_Log.cxx LIBRARIES Hist Gpad Graf Core) diff --git a/hist/hist/test/test_TScatter_Log.cxx b/hist/hist/test/test_TScatter_Log.cxx new file mode 100644 index 0000000000000..2e9db22cf9fd2 --- /dev/null +++ b/hist/hist/test/test_TScatter_Log.cxx @@ -0,0 +1,51 @@ +// Unit tests for TScatter and TScatter2D log scale rendering (Issue #23833) + +#include "gtest/gtest.h" + +#include "TROOT.h" +#include "TCanvas.h" +#include "TScatter.h" +#include "TScatter2D.h" + +TEST(TScatterLog, NonPositivePointsInLogScale) +{ + gROOT->SetBatch(true); + auto c = new TCanvas("c_scatter_log", "c_scatter_log", 800, 600); + c->SetLogy(); + + // 5 points where index 2 has non-positive y = 0 + const int n = 5; + double x[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; + double y[n] = {10.0, 20.0, 0.0, 40.0, 50.0}; + double col[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; + double size[n] = {2.0, 2.0, 2.0, 2.0, 2.0}; + + auto scatter = new TScatter(n, x, y, col, size); + scatter->Draw("A"); + c->Update(); + + EXPECT_NE(scatter->GetHistogram(), nullptr); + delete c; +} + +TEST(TScatter2DLog, NonPositivePointsInLogScale) +{ + gROOT->SetBatch(true); + auto c = new TCanvas("c_scatter2d_log", "c_scatter2d_log", 800, 600); + c->SetLogz(); + + // 5 points where index 2 has non-positive z = 0 + const int n = 5; + double x[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; + double y[n] = {10.0, 20.0, 30.0, 40.0, 50.0}; + double z[n] = {1.0, 2.0, 0.0, 4.0, 5.0}; + double col[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; + double size[n] = {2.0, 2.0, 2.0, 2.0, 2.0}; + + auto scatter2d = new TScatter2D(n, x, y, z, col, size); + scatter2d->Draw(); + c->Update(); + + EXPECT_NE(scatter2d->GetHistogram(), nullptr); + delete c; +} diff --git a/hist/histpainter/src/TGraphPainter.cxx b/hist/histpainter/src/TGraphPainter.cxx index 2199dbd7d4bd7..b7870d1391b27 100644 --- a/hist/histpainter/src/TGraphPainter.cxx +++ b/hist/histpainter/src/TGraphPainter.cxx @@ -4497,13 +4497,13 @@ void TGraphPainter::PaintScatter(TScatter *theScatter, Option_t* chopt) if (theColor || theSize) theScatter->TAttMarker::Modify(); if (logx) { if (theX[i]>0) x = log10(theX[i]); - else break; + else continue; } else { x = theX[i]; } if (logy) { if (theY[i]>0) y = log10(theY[i]); - else break; + else continue; } else { y = theY[i]; } @@ -4731,19 +4731,19 @@ void TGraphPainter::PaintScatter2D(TScatter2D *theScatter, Option_t* chopt) theScatter->TAttMarker::Modify(); if (logx) { if (theX[i]>0) x = log10(theX[i]); - else break; + else continue; } else { x = theX[i]; } if (logy) { if (theY[i]>0) y = log10(theY[i]); - else break; + else continue; } else { y = theY[i]; } if (logz) { if (theZ[i]>0) z = log10(theZ[i]); - else break; + else continue; } else { z = theZ[i]; } From f93160cd3296442febd0cc52ccdb446412bc3ce5 Mon Sep 17 00:00:00 2001 From: Muntasir Mamun Date: Sun, 25 Oct 2026 21:30:27 +0600 Subject: [PATCH 2/4] [test] Allocate test objects on the stack and rename test to test_tscatter --- hist/hist/test/CMakeLists.txt | 2 +- ...est_TScatter_Log.cxx => test_tscatter.cxx} | 26 +++++++++---------- 2 files changed, 13 insertions(+), 15 deletions(-) rename hist/hist/test/{test_TScatter_Log.cxx => test_tscatter.cxx} (65%) diff --git a/hist/hist/test/CMakeLists.txt b/hist/hist/test/CMakeLists.txt index 030f73d4ef353..b42bee50b7a99 100644 --- a/hist/hist/test/CMakeLists.txt +++ b/hist/hist/test/CMakeLists.txt @@ -62,4 +62,4 @@ ROOT_ADD_GTEST(test_projections test_projections.cxx LIBRARIES Hist) ROOT_ADD_GTEST(test_th1d_tpad_buildlegend test_th1d_tpad_buildlegend.cxx LIBRARIES Hist Gpad) # Test for https://github.com/root-project/root/issues/23833 -ROOT_ADD_GTEST(test_TScatter_Log test_TScatter_Log.cxx LIBRARIES Hist Gpad Graf Core) +ROOT_ADD_GTEST(test_tscatter test_tscatter.cxx LIBRARIES Hist Gpad Graf Core) diff --git a/hist/hist/test/test_TScatter_Log.cxx b/hist/hist/test/test_tscatter.cxx similarity index 65% rename from hist/hist/test/test_TScatter_Log.cxx rename to hist/hist/test/test_tscatter.cxx index 2e9db22cf9fd2..1bac83ce16713 100644 --- a/hist/hist/test/test_TScatter_Log.cxx +++ b/hist/hist/test/test_tscatter.cxx @@ -10,8 +10,8 @@ TEST(TScatterLog, NonPositivePointsInLogScale) { gROOT->SetBatch(true); - auto c = new TCanvas("c_scatter_log", "c_scatter_log", 800, 600); - c->SetLogy(); + TCanvas c("c_scatter_log", "c_scatter_log", 800, 600); + c.SetLogy(); // 5 points where index 2 has non-positive y = 0 const int n = 5; @@ -20,19 +20,18 @@ TEST(TScatterLog, NonPositivePointsInLogScale) double col[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; double size[n] = {2.0, 2.0, 2.0, 2.0, 2.0}; - auto scatter = new TScatter(n, x, y, col, size); - scatter->Draw("A"); - c->Update(); + TScatter scatter(n, x, y, col, size); + scatter.Draw("A"); + c.Update(); - EXPECT_NE(scatter->GetHistogram(), nullptr); - delete c; + EXPECT_NE(scatter.GetHistogram(), nullptr); } TEST(TScatter2DLog, NonPositivePointsInLogScale) { gROOT->SetBatch(true); - auto c = new TCanvas("c_scatter2d_log", "c_scatter2d_log", 800, 600); - c->SetLogz(); + TCanvas c("c_scatter2d_log", "c_scatter2d_log", 800, 600); + c.SetLogz(); // 5 points where index 2 has non-positive z = 0 const int n = 5; @@ -42,10 +41,9 @@ TEST(TScatter2DLog, NonPositivePointsInLogScale) double col[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; double size[n] = {2.0, 2.0, 2.0, 2.0, 2.0}; - auto scatter2d = new TScatter2D(n, x, y, z, col, size); - scatter2d->Draw(); - c->Update(); + TScatter2D scatter2d(n, x, y, z, col, size); + scatter2d.Draw(); + c.Update(); - EXPECT_NE(scatter2d->GetHistogram(), nullptr); - delete c; + EXPECT_NE(scatter2d.GetHistogram(), nullptr); } From 0c65f14dcde651080f3d1c8898568c8404213dc3 Mon Sep 17 00:00:00 2001 From: Muntasir Mamun Date: Mon, 26 Oct 2026 00:00:26 +0600 Subject: [PATCH 3/4] [test] Set Z-axis range in TScatter test and format with clang-format --- hist/hist/test/test_tscatter.cxx | 1 + hist/histpainter/src/TGraphPainter.cxx | 15 ++++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/hist/hist/test/test_tscatter.cxx b/hist/hist/test/test_tscatter.cxx index 1bac83ce16713..5623d9222c61b 100644 --- a/hist/hist/test/test_tscatter.cxx +++ b/hist/hist/test/test_tscatter.cxx @@ -21,6 +21,7 @@ TEST(TScatterLog, NonPositivePointsInLogScale) double size[n] = {2.0, 2.0, 2.0, 2.0, 2.0}; TScatter scatter(n, x, y, col, size); + scatter.GetZaxis()->SetRangeUser(1.0, 5.0); scatter.Draw("A"); c.Update(); diff --git a/hist/histpainter/src/TGraphPainter.cxx b/hist/histpainter/src/TGraphPainter.cxx index b7870d1391b27..e1e6ca5fb413d 100644 --- a/hist/histpainter/src/TGraphPainter.cxx +++ b/hist/histpainter/src/TGraphPainter.cxx @@ -4497,13 +4497,15 @@ void TGraphPainter::PaintScatter(TScatter *theScatter, Option_t* chopt) if (theColor || theSize) theScatter->TAttMarker::Modify(); if (logx) { if (theX[i]>0) x = log10(theX[i]); - else continue; + else + continue; } else { x = theX[i]; } if (logy) { if (theY[i]>0) y = log10(theY[i]); - else continue; + else + continue; } else { y = theY[i]; } @@ -4731,19 +4733,22 @@ void TGraphPainter::PaintScatter2D(TScatter2D *theScatter, Option_t* chopt) theScatter->TAttMarker::Modify(); if (logx) { if (theX[i]>0) x = log10(theX[i]); - else continue; + else + continue; } else { x = theX[i]; } if (logy) { if (theY[i]>0) y = log10(theY[i]); - else continue; + else + continue; } else { y = theY[i]; } if (logz) { if (theZ[i]>0) z = log10(theZ[i]); - else continue; + else + continue; } else { z = theZ[i]; } From 0517a41ea85ffdc228704fe5726190473d516fc9 Mon Sep 17 00:00:00 2001 From: Muntasir Mamun Date: Mon, 26 Oct 2026 19:54:44 +0600 Subject: [PATCH 4/4] [test] Exclude unit test per reviewer request --- hist/hist/test/CMakeLists.txt | 3 -- hist/hist/test/test_tscatter.cxx | 50 -------------------------------- 2 files changed, 53 deletions(-) delete mode 100644 hist/hist/test/test_tscatter.cxx diff --git a/hist/hist/test/CMakeLists.txt b/hist/hist/test/CMakeLists.txt index b42bee50b7a99..7fa26b0a8f1ba 100644 --- a/hist/hist/test/CMakeLists.txt +++ b/hist/hist/test/CMakeLists.txt @@ -60,6 +60,3 @@ ROOT_ADD_TEST(test-tdirectoryfile_destructor_segfault COMMAND tdirectoryfile_des ROOT_ADD_GTEST(test_projections test_projections.cxx LIBRARIES Hist) ROOT_ADD_GTEST(test_th1d_tpad_buildlegend test_th1d_tpad_buildlegend.cxx LIBRARIES Hist Gpad) - -# Test for https://github.com/root-project/root/issues/23833 -ROOT_ADD_GTEST(test_tscatter test_tscatter.cxx LIBRARIES Hist Gpad Graf Core) diff --git a/hist/hist/test/test_tscatter.cxx b/hist/hist/test/test_tscatter.cxx deleted file mode 100644 index 5623d9222c61b..0000000000000 --- a/hist/hist/test/test_tscatter.cxx +++ /dev/null @@ -1,50 +0,0 @@ -// Unit tests for TScatter and TScatter2D log scale rendering (Issue #23833) - -#include "gtest/gtest.h" - -#include "TROOT.h" -#include "TCanvas.h" -#include "TScatter.h" -#include "TScatter2D.h" - -TEST(TScatterLog, NonPositivePointsInLogScale) -{ - gROOT->SetBatch(true); - TCanvas c("c_scatter_log", "c_scatter_log", 800, 600); - c.SetLogy(); - - // 5 points where index 2 has non-positive y = 0 - const int n = 5; - double x[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; - double y[n] = {10.0, 20.0, 0.0, 40.0, 50.0}; - double col[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; - double size[n] = {2.0, 2.0, 2.0, 2.0, 2.0}; - - TScatter scatter(n, x, y, col, size); - scatter.GetZaxis()->SetRangeUser(1.0, 5.0); - scatter.Draw("A"); - c.Update(); - - EXPECT_NE(scatter.GetHistogram(), nullptr); -} - -TEST(TScatter2DLog, NonPositivePointsInLogScale) -{ - gROOT->SetBatch(true); - TCanvas c("c_scatter2d_log", "c_scatter2d_log", 800, 600); - c.SetLogz(); - - // 5 points where index 2 has non-positive z = 0 - const int n = 5; - double x[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; - double y[n] = {10.0, 20.0, 30.0, 40.0, 50.0}; - double z[n] = {1.0, 2.0, 0.0, 4.0, 5.0}; - double col[n] = {1.0, 2.0, 3.0, 4.0, 5.0}; - double size[n] = {2.0, 2.0, 2.0, 2.0, 2.0}; - - TScatter2D scatter2d(n, x, y, z, col, size); - scatter2d.Draw(); - c.Update(); - - EXPECT_NE(scatter2d.GetHistogram(), nullptr); -}