[graph] Do not abort scatter painting loop on non-positive log coordinates (fixes #23833) by muntasir-396 · Pull Request #23834 · root-project/root

muntasir-396 · 2026-10-25T07:26:14Z

Summary

When plotting a TScatter or TScatter2D on a pad with logarithmic scale enabled (gPad->SetLogx(), SetLogy(), or SetLogz()), encountering any non-positive coordinate (x <= 0, y <= 0, or z <= 0) caused TGraphPainter::PaintScatter and PaintScatter2D to terminate the entire rendering loop via else break;.

As a consequence, if an early point in the array happened to be non-positive, all subsequent valid positive points were omitted from the canvas.

Changes

In TGraphPainter::PaintScatter and PaintScatter2D, replaced else break; with else continue; in logx, logy, and logz marker loops.
Added regression unit tests in hist/hist/test/test_TScatter_Log.cxx testing TScatter and TScatter2D with non-positive points under logarithmic scales.

Fixes #23833.

…nates (fixes root-project#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.

ferdymercury

Does the same issue appear with TH2D drawn with COLZ with logz?
Thks

muntasir-396 · 2026-10-25T07:51:20Z

Hi @ferdymercury

Thanks for the review!

I checked THistPainter::PaintColorLevels and PaintColorLevelsFast for TH2D histograms drawn with COLZ and logz. The issue doesn’t seem to occur there. THistPainter handles non-positive bins correctly using continue; and clamping (z = z > 0 ? TMath::Log10(z) : zmin;), so the loop continues instead of stopping early, and all bins are painted as expected.

The premature else break; issue was specific to the marker loops in TGraphPainter::PaintScatter and PaintScatter2D.

dpiparo

Thanks for this PR. Could you please address the comments so that we can start the integration process?

dpiparo · 2026-10-25T09:16:18Z

 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)


can we call this test_tscatter and test_tscatter.cxx?

dpiparo · 2026-10-25T09:16:38Z

+TEST(TScatterLog, NonPositivePointsInLogScale)
+{
+   gROOT->SetBatch(true);
+   auto c = new TCanvas("c_scatter_log", "c_scatter_log", 800, 600);


can we put this on the stack?

dpiparo · 2026-10-25T09:17:14Z

+TEST(TScatter2DLog, NonPositivePointsInLogScale)
+{
+   gROOT->SetBatch(true);
+   auto c = new TCanvas("c_scatter2d_log", "c_scatter2d_log", 800, 600);


same here, on the stack

dpiparo · 2026-10-25T09:17:20Z

+   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);


same here, on the stack

dpiparo · 2026-10-25T09:19:10Z

@muntasir-396 , did you use AI for this contribution (see https://github.com/root-project/root/blob/master/CONTRIBUTING.md#ai-assisted-coding-disclosure)?

…atter

muntasir-396 · 2026-10-25T15:33:13Z

Yes @dpiparo ,, I used an AI assistant to help draft the issue description, reproducer macro, and PR text, in compliance with the disclosure policy.

I have reviewed and verified the underlying C++ fix in TGraphPainter.cxx (replacing else break; with else continue; so non-positive points in log scale don't prematurely abort the rendering loop).

I have also addressed your review comments:

Renamed the test file and CMake target to test_tscatter.
Changed all test objects (TCanvas, TScatter, TScatter2D) to be allocated on the stack instead of using new.

The updated commit has been pushed to the branch. Thank you for the guidance!

dpiparo · 2026-10-25T17:42:01Z

thanks. Let's how the tests go.

muntasir-396 · 2026-10-25T18:01:24Z

Updated in commit 0c65f14:

Formatted hist/histpainter/src/TGraphPainter.cxx with git-clang-format to strictly follow ROOT's clang-format conventions.
Set the Z-axis range on TScatter in test_tscatter.cxx (scatter.GetZaxis()->SetRangeUser(1.0, 5.0)) so the underlying palette histogram validation in PaintScatter passes cleanly without diagnostics.

Also note: the earlier clang-format CI check failure was caused by apt.llvm.org returning 404 for Ubuntu 26.04 (resolute) in llvm.sh 20 on GitHub's ubuntu-latest runner. Pinning runs-on: ubuntu-24.04 in .github/workflows/code_analysis.yml upstream avoids that runner issue.

github-actions · 2026-10-25T22:11:11Z

Test Results

0 tests 0 ✅ 0s ⏱️
0 suites 0 💤
0 files 0 ❌

Results for commit 0c65f14.

linev

Provided solution incomplete. break in the code in some places was intentional while produced output will be corrupted anyway. In case of negative values on log axes ranges not correctly calculated.

Therefore one either fixes algorithm how ranges are defined (preferable way) or just print error message when negative value appears on the log scale. Only in the first case one then replace break by continue

Just to explain my comment - if color scale in TScatter2D is logarithmic then negative values made complete output incorrect. But seems to be X, Y and Z values handled correctly. So you only should keep replacement of break by continue for X,Y and Z coordinates.

Provided tests must be excluded - they test nothing. TScatter::GetHistogram() always return valid histogram pointer - disregard of problems during painting. Real graphics tests should be implemented in stressGraphics.cxx. There we missing test of TScatter2D for instance.

muntasir-396 requested review from bellenot, guitargeek and hageboeck as code owners October 25, 2026 07:26

ferdymercury approved these changes Oct 25, 2026

View reviewed changes

dpiparo requested changes Oct 25, 2026

View reviewed changes

[test] Allocate test objects on the stack and rename test to test_tsc…

f93160c

…atter

dpiparo added AI-Assisted pr:squash on merge labels Oct 25, 2026

[test] Set Z-axis range in TScatter test and format with clang-format

0c65f14

linev self-assigned this Oct 26, 2026

linev self-requested a review October 26, 2026 06:22

linev requested changes Oct 26, 2026

View reviewed changes

Conversation

muntasir-396 commented Oct 25, 2026

Summary

Changes

Uh oh!

ferdymercury left a comment

Choose a reason for hiding this comment

Uh oh!

muntasir-396 commented Oct 25, 2026

Uh oh!

dpiparo left a comment

Choose a reason for hiding this comment

Uh oh!

dpiparo Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

dpiparo Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dpiparo Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

dpiparo Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

dpiparo commented Oct 25, 2026

Uh oh!

muntasir-396 commented Oct 25, 2026

Uh oh!

dpiparo commented Oct 25, 2026

Uh oh!

muntasir-396 commented Oct 25, 2026

Uh oh!

github-actions Bot commented Oct 25, 2026

Test Results

Uh oh!

linev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linev left a comment •

edited

Loading