-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[hist] Add SetRefPad for consistent axis scaling #20770
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 20 files 20 suites 3d 16h 7m 55s ⏱️ For more details on these failures, see this check. Results for commit d1c338c. |
hist/hist/inc/TAxis.h
Outdated
| void SetRefPad(TVirtualPad *pad); | ||
| Float_t GetRefLength() const { return fRefLength; } | ||
|
|
||
| ClassDefOverride(TAxis,11) //Axis class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to increment the class version here: the only data member you added to the class was fRefLength, which is not taking part in the I/O. This is also called a "transient" data member, and these are marked by the exclamation mark in the associated doc string.
What made you increase the class version in your most recent commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification regarding the transient member. I initially kept the ClassDef version unchanged for that exact reason.
However, I later bumped it because the CI failed on roottest-root-aclic-offset-offset with a size mismatch error (Expected: 1000, Got: 1024). I misinterpreted this failure as the system rejecting the class layout change, which is why I attempted the version bump to fix it.
I have now re-analyzing the test output, I realize the failure is simply detecting the 24-byte size increase (3 axes × 8 bytes padding/float) caused by adding fRefLength.
I have reverted the ClassDef change in the latest commit. Could you advise on how to handle the roottest failure? Does the reference file (offset.ref) need to be updated to reflect the new TH1 size?
Thanks!
This Pull request:
Implement opt-in Reference Pad scaling for TAxis (Fixes #20484)
Changes or fixes:
This PR addresses the long-standing issue of inconsistent axis title/label sizing and offsetting between pads of different sizes (e.g., in ratio plots), as described in issue #20484.
The Problem:
When creating canvases with split pads (e.g., 70/30 split), the default behavior scales text sizes relative to the pad height. This results in tiny, unreadable text in the smaller auxiliary pad, requiring users to manually tune
SetLabelSizeandSetTitleOffsetfor every axis.The Solution:
I have implemented an opt-in mechanism that allows an axis to use the dimensions of a "Reference Pad" for scaling calculations.
TAxis::SetRefPad(TVirtualPad *pad).fRefLengthtoTAxisto store the reference height (avoiding unsafe pointer storage).TGaxis::PaintAxisto check iffRefLength > 0.PaintAxiscalculates a scaling factor (fRefLength / currentPadHeight) and temporarily applies it tofLabelSize,fTitleSize,fTickSize,fLabelOffset, andfTitleOffsetusing anAttributeRestorer(RAII) pattern.Design Choice:
I chose an opt-in approach (
SetRefPad) rather than automatic scaling to preserve backward compatibility.Note:
I also investigated applying this fix to
TPaveStatsandTLegend, but due to the complexity of their internal layout logic, those changes are not included in this PR. This PR focuses strictly on fixing theTAxisconsistency, which was the core request of the issue.Verification Code
The following macro demonstrates the fix on a standard ratio plot.
(Please refer to the attached image for the output)
Checklist:
This PR fixes #20484