FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix axisartist label font sizes set through Axes by baba9811 · Pull Request #32384 · matplotlib/matplotlib · GitHub

Repository navigation

Fix axisartist label font sizes set through Axes - #32384

Open
baba9811 wants to merge 4 commits into
matplotlib:mainfrom
baba9811:fix/axisartist-label-fontsize
Open

baba9811 wants to merge 4 commits into
matplotlib:mainfrom
baba9811:fix/axisartist-label-fontsize

Conversation

Copy link
Copy Markdown

PR summary

Fixes #28124.

Make axisartist axis labels follow the font size set through set_xlabel and set_ylabel. Direct font settings on individual axisartist labels still take precedence. Font lookup caches store snapshots so inherited font properties do not retain closed figures.

Regression tests compare rendered figures and cover explicit overrides, mutable font properties, copying, bounding boxes, and figure lifetime. The previous attempt in #31155 is closed and unmerged.

Testing

  • Axisartist, axes_grid1, Text, and font-manager suites: 276 passed, 52 skipped. Skips require Ghostscript, Inkscape, TeX, unavailable fonts, or another operating system.
  • The new PNG comparisons failed before the fix and pass afterward. The original public-API reproduction now renders the requested 20pt labels on both axes.
  • Additional SVG output comparison and PDF raster comparison with Poppler both match the explicit-label workaround.
  • Changed-file prek checks, including mypy, Ruff, and codespell, pass.
  • The full repository suite and cross-platform CI have not been run locally.

AI Disclosure

Used Codex for implementation and testing.

Copy link
Copy Markdown

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process.

You can also join us on discourse chat for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide.
Please let us know if (and how) you use AI, it will help us give you better feedback on your PR.

We strive to be a welcoming and open project. Please follow our Code of Conduct.

rcomer commented Sep 24, 2026

Copy link
Copy Markdown
Member

Hi @baba9811

Used Codex for implementation and testing.

This reads like the contribution is fully AI generated. Please review our AI Policy and confirm whether there is a human author who has thought through the problem and can discuss the change with us.

Copy link
Copy Markdown
Author

@rcomer
Hi, I directed the investigation, reviewed the changes, and can discuss the implementation. I used Codex for implementation and testing. My original disclosure didn’t clearly explain my involvement.

The issue is that axisartist draws a separate label whose font size wasn’t following set_xlabel / set_ylabel. The fix makes it inherit that size while preserving explicit settings on the axisartist label. The font-cache change stores a snapshot to avoid retaining closed figures

baba9811 force-pushed the fix/axisartist-label-fontsize branch from 8d59420 to c80b899 Compare October 3, 2026 21:01

timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have the impression that "I directed the investigation, reviewed the changes," was not done carefully and deep enough. This is an operational fix to the reported problem. But is it the correct solution to the underlying problem? The underlying issue is that axisartist defines its own axis and label independent of base Axes.axis.label. Is this a sufficient overall solution or are just starting whack-a-mole to keep the objects synced?

Comment on lines +1476 to +1478
if isinstance(prop, FontProperties):
# Cache a snapshot, which cannot change or retain a reference artist.
prop = prop.copy()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is not the responsibility of findfont. It belongs in _findfont_cached.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The snapshot must be taken before lru_cache builds its key. Copying inside the decorated function still retains the original, reference-bearing object; I reproduced the figure remaining alive until the cache was cleared.

I kept this in findfont, the sole production caller, which already prepares the other cache-key inputs. An uncached private wrapper would also be correct, but would add another method and cache-invalidation changes without changing the supported behavior.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fair point. It's a bit unfortunate that this is needed outside the cache, but the FontProperties conversion (prop = FontProperties._from_any(prop)) is inside, which we want to keep for speed.

I guess we have to live with the compromize of split responsibility.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I looked into the wrapper option a bit more and tried a small uncached wrapper around _findfont_cached. It keeps the snapshot before cache-key creation and the _from_any conversion inside the cached worker. Cache invalidation can also stay unchanged, so my earlier comment overstated the changes needed.
This still splits the two steps, but keeps the snapshot handling next to the cache implementation rather than in findfont. Would you prefer that small separation, or keeping the current placement?

Comment on lines +1059 to +1061
if "labelsize" not in kwargs:
self.label._fontproperties = _AxisLabelFontProperties(
self.label.get_fontproperties(), self.axis.label)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What motivates the archtectural decision to reference self.axis.label? Have you considered other solution strategies?

If this approach is the right way forward it needs concise documentation what you do (semantically not technically) and why. Also, such a behavior should not be patched onto the label, but should be implmented as capability of the label.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

self.axis.label already supplies the default text and color, so using it for the default font size follows the same behavior. Each axisartist label can still set its own size.

Overriding get_fontsize alone isn't enough here: Text uses _fontproperties directly for layout and drawing. Artist callbacks also don't catch changes made through get_fontproperties().set_size(…). That's why I kept the size lookup in FontProperties.

I agree that AxisLabel should own the setup. The local revision moves it there with set_fontsize("auto") and documents how setting a size or replacing the font properties stops inheritance. TickLabels keeps its existing behavior.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Technically, AxisLabel currently works with self.axis None. However, real use cases should always have self.axis set. If we could rely on that, would it be an option to always delegate font properties lookup to self.axis.label so that we don't have to maintain state in AxisLabel? This is turn would alleviate the need for _AxisLabelFontProperties.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I checked this against main. Left and right labels, as well as floating labels using the same Axis, can currently have independent font settings. Always using the reference label's FontProperties would make their setters modify the same object.

I'd prefer to preserve those local settings for this fix. Did you mean to delegate defaults while keeping local overrides, or to share font settings including edits? Keeping overrides would still require some per-label state, although it need not use _AxisLabelFontProperties.

baba9811 commented Oct 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

@timhoffm
I'd keep the labels separate because each side or floating axis needs its own position, direction, and styling.
On the whack-a-mole point, this doesn't copy a size between two labels whenever something changes. The axisartist label reads the reference label's current size until a local size is set or its font properties are replaced. Layout and drawing use that same lookup, so setters and direct font-property changes don't need separate synchronization patches. That's why I think this is a reasonable fix for the font-size issue within the existing design.
I agree that the setup belongs in AxisLabel, and the revision moves it there. The focused tests report 284 passed and 52 skipped for missing tools, fonts, or platform restrictions. The changed file checks also pass.

This branch has not been deployed

No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: can not set ylabel fontsize in host_subplot

4 participants


Back | FazBrowse Home | New Git URL