Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

MNT: Registered 3rd party scales do not need an axis parameter anymore #29358

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
Loading
from

Conversation

timhoffm
Copy link
Member

First step of #29349.

@timhoffm timhoffm marked this pull request as ready for review December 30, 2024 00:35
@timhoffm timhoffm added this to the v3.11.0 milestone Dec 30, 2024
@rcomer
Copy link
Member

rcomer commented Jan 5, 2025

Should the Custom Scale example also be updated at this point? Its __init__ method calls the parent __init__ method, which still requires axis. However, I think it could just not call the parent method (several of the built in scales do not).

@rcomer
Copy link
Member

rcomer commented Jan 5, 2025

I think the note here also needs updating

Notes
-----
The following note is for scale implementers.
For back-compatibility reasons, scales take an `~matplotlib.axis.Axis`
object as first argument. However, this argument should not
be used: a single scale object should be usable by multiple
`~matplotlib.axis.Axis`\es at the same time.

@timhoffm
Copy link
Member Author

timhoffm commented Jan 5, 2025

@rcomer This commit/PR is one step in removing the parameter (see #29349). The points you bring up have to be addressed in the course of making the parameter fully optional. I believe it's best to address them after making the axis parameter in __init__ optional (step 3 #29349). I will do that, but I'm undecided whether it's better as part of this PR or easier to review separately. Technically, the aspects (i) it's possible to use/create scale classes without the axis parameter and (ii) the registry can use classes with and without the axis parameter are independent - which could suggest making separate PRs for easier review. OTOH practical use is only possible after both have been implemented, which could suggest putting both in to one PR.

Specific note on:

However, I think it could just not call the parent method (several of the built in scales do not).

I'd rather not go this way. The current ScaleBase.__init__ is empty, so it does not matter. But not calling super init would break subclasses if we decide later that we need ScaleBase.__init__. Not calling is okish for our own classes, which we have control over and can update if needed. But I would not recommend that for third-party subclasses. Since we can easily prevent that hassle through #29349 step 3, I would take that route.

@rcomer
Copy link
Member

rcomer commented Jan 5, 2025

Could this PR wait till after step 3?

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.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.