GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Refactoring: Removing axis parameter from scales by vagnermcj · Pull Request #29988 · matplotlib/matplotlib

Conversation

@vagnermcj

@vagnermcj vagnermcj commented Apr 29, 2025

Copy link
Copy Markdown
Contributor

This PR addresses issue #29349 by refactoring built-in scale classes to make the axis parameter optional. The goal is to support future removal of axis while maintaining backward compatibility with current usage.

To achieve this, a decorator was introduced to allow constructors to accept or ignore axis. This was applied across all built-in scales. The scale_factory() function was updated to handle scale classes that do or do not use axis, and register_scale() now issues a deprecation warning if the constructor still expects it.

These changes prepare the codebase for eventual deprecation of axis in scale initializers, while avoiding any breaking changes in current usage.

My only doubt is which initializers for the scales should have the parameter as "axis = None". If all of them should have it, then i have to change the way my decorator works because axis could change position in parameters index

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@github-actions github-actions Bot added the topic: transforms and scales label Apr 29, 2025
@vagnermcj
vagnermcj marked this pull request as draft April 29, 2025 20:02
github-actions[bot]
github-actions Bot reviewed Apr 29, 2025
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

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.

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.

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

For details on testing, writing docs, and our review process, please see the developer guide

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vagnermcj
vagnermcj marked this pull request as ready for review April 30, 2025 13:12
timhoffm
timhoffm reviewed May 13, 2025
View reviewed changes

@timhoffm timhoffm left a comment

Copy link
Copy Markdown
Member

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 PR. This is a bit tricky to get right in a backward-compatible and migration-easy way. I think we need to do it slightly differently.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vagnermcj

vagnermcj commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

Apologies for the sequence of commits — I wanted to ensure each feedback point was properly addressed and tested incrementally. Please let me know if this is the appropriate way to validate ruff, mypy-stubtest, and CI tests in general, or if you'd prefer a cleaner commit history or a different workflow.

I changed back the deprecation warning function, and changed the decorator as well. However, in the decorator i had many errors when trying to check "if args and isinstance(args[0], ScaleBase):", so instead of using ScaleBase as a reference i changed to "if args and isinstance(args[0], mpl.axis.Axis):". Does it make sense?

Let me know if anything else needs to be refined or if the structure of the changes could be improved further. Thanks again for your time and guidance throughout the review!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

timhoffm
timhoffm reviewed May 21, 2025
View reviewed changes

@timhoffm timhoffm left a comment

Copy link
Copy Markdown
Member

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.

Apologies for the sequence of commits — I wanted to ensure each feedback point was properly addressed and tested incrementally. Please let me know if this is the appropriate way to validate ruff, mypy-stubtest, and CI tests in general, or if you'd prefer a cleaner commit history or a different workflow.

It does not matter for small PRs. We'll typically review the full diff and squash-merge.

However, in the decorator i had many errors when trying to check "if args and isinstance(args[0], ScaleBase):", so instead of using ScaleBase as a reference i changed to "if args and isinstance(args[0], mpl.axis.Axis):". Does it make sense?

Yes. Sorry my bad for the incorrect suggestion.

Let me know if anything else needs to be refined or if the structure of the changes could be improved further. Thanks again for your time and guidance throughout the review!

You should add a test that the decorated scales can be created with and without axis passed. - Either test on one of the scales, or write an explicit test for the decorator.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@melissawm melissawm added this to First Time Contributors May 22, 2025
@melissawm melissawm moved this to Waiting for author in First Time Contributors May 22, 2025
@vagnermcj

vagnermcj commented May 29, 2025

Copy link
Copy Markdown
Contributor Author

image

I started having this error with linting, but i don't understand why. It seems the error is in the _mathtext.py file, but i never changed it

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rcomer

rcomer commented May 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Looks like mypy just made a new release, so I suspect we will now see the same linting errors on all PRs. You can ignore them here.

vagnermcj reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vagnermcj
vagnermcj requested a review from timhoffm June 6, 2025 02:15
timhoffm
timhoffm reviewed Jun 6, 2025
View reviewed changes

@timhoffm timhoffm left a comment

Copy link
Copy Markdown
Member

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.

Please leave out the unrelated typing modifications.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

timhoffm
timhoffm reviewed Jun 7, 2025
View reviewed changes

@timhoffm timhoffm left a comment

Copy link
Copy Markdown
Member

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.

This is good, except for one annotation to self.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/matplotlib/scale.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

timhoffm
timhoffm approved these changes Jun 7, 2025
View reviewed changes
@vagnermcj

vagnermcj commented Jun 24, 2025

Copy link
Copy Markdown
Contributor Author

Do i still need to make some change in the code? I see that it was accepted but some tests are failing

It's my first PR so i'm not sure 😅

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timhoffm timhoffm added the status: needs review label Jun 25, 2025
@timhoffm

timhoffm commented Jun 25, 2025

Copy link
Copy Markdown
Member

We require two reviews from core developers. The second one is still pending. IIRC the test failures were unrelated, but since the test runs have been cleaned up, you could rebase onto main and force-push.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

QuLogic
QuLogic reviewed Jun 25, 2025
View reviewed changes
Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

vagnermcj added 11 commits June 25, 2025 21:21
@vagnermcj
Adding a decorator and Refactoring functions
47ae63b
@vagnermcj
Fixing Ruff Errors
5c5ddd2
@vagnermcj
Update scale.pyi
b770f6d
@vagnermcj
Adding new line to the end of scale.pyi
16cef9d
@vagnermcj
Update in docstring
698c35f
@vagnermcj
Fixing Handle Function
8a0d3c7
@vagnermcj
Support optional axis in scales
11c9bb8
Updated my refactor based on the feedbacks received
@vagnermcj
Fixing ruff error
25bdfc2
@vagnermcj
change in parameters and in decorator
00214f6
@vagnermcj
parameter fix
4cf2dde
@vagnermcj
minor change in pyi
2e691c6
vagnermcj and others added 12 commits June 25, 2025 21:21
@vagnermcj @timhoffm
Update lib/matplotlib/scale.py
618054d
Co-authored-by: Tim Hoffmann <[email protected]>
@vagnermcj @timhoffm
Update lib/matplotlib/scale.py
a1002b3
Co-authored-by: Tim Hoffmann <[email protected]>
@vagnermcj @timhoffm
Update lib/matplotlib/scale.py
2ea3468
Co-authored-by: Tim Hoffmann <[email protected]>
@vagnermcj @timhoffm
Update lib/matplotlib/scale.pyi
0e7dbc5
Co-authored-by: Tim Hoffmann <[email protected]>
@vagnermcj
Updating self and axis in pyi
fa15c3a
@vagnermcj
returning scale_factory to default
d9f5722
@vagnermcj
Ruff checks
8aebcc0
@vagnermcj
description fix
6da5188
@vagnermcj @timhoffm
Update lib/matplotlib/scale.pyi
9b7cce4
Co-authored-by: Tim Hoffmann <[email protected]>
@vagnermcj
Undoing Unrelated Modifications
9c9101e
@vagnermcj
fixing mypy tests
5d7303f
@vagnermcj @timhoffm
Update lib/matplotlib/scale.pyi
dabac62
Co-authored-by: Tim Hoffmann <[email protected]>
@vagnermcj
vagnermcj force-pushed the refactor-for-issue-29349 branch from 223dc30 to dabac62 Compare June 26, 2025 00:23
QuLogic
QuLogic reviewed Jun 26, 2025
View reviewed changes
Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vagnermcj @QuLogic
keyword-argument suggestion
75896a2
Co-authored-by: Elliott Sales de Andrade <[email protected]>
QuLogic
QuLogic reviewed Jun 26, 2025
View reviewed changes
Comment thread lib/matplotlib/scale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vagnermcj @QuLogic
kwargs pop before function call
bd8a135
Co-authored-by: Elliott Sales de Andrade <[email protected]>
QuLogic
QuLogic approved these changes Jun 27, 2025
View reviewed changes
@QuLogic QuLogic removed the status: needs review label Jun 27, 2025
@QuLogic
QuLogic merged commit c78c2f4 into matplotlib:main Jun 27, 2025
@QuLogic QuLogic added this to the v3.11.0 milestone Jun 27, 2025
@QuLogic QuLogic moved this from Waiting for author to Merged in First Time Contributors Jun 27, 2025
@QuLogic

QuLogic commented Jun 27, 2025

Copy link
Copy Markdown
Member

Thanks @vagnermcj! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

vagnermcj reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 7, 2025
DOC: Scale axis parameter #30404
Merged
[MNT]: Remove axis parameter from scales #29349
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@QuLogic QuLogic QuLogic approved these changes
@timhoffm timhoffm timhoffm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic: transforms and scales

Projects

First Time Contributors
Status: Merged

Milestone

v3.11.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vagnermcj @rcomer @timhoffm @QuLogic
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.