Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Sign up
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 2 commits intomatplotlib:mainfromtimhoffm:scale-axis
Aug 14, 2025

Conversation

@timhoffm
Copy link
Member

First step of#29349.

@timhoffmtimhoffmforce-pushed thescale-axis branch 4 times, most recently from70fc0eb tobb30befCompareDecember 30, 2024 00:03
@timhoffmtimhoffm marked this pull request as ready for reviewDecember 30, 2024 00:35
@timhoffmtimhoffm added this to thev3.11.0 milestoneDec 30, 2024
@rcomer
Copy link
Member

Should theCustom Scale example also be updated at this point? Its__init__ method calls the parent__init__ method, which still requiresaxis. However, I think it could just not call the parent method (several of the built in scales do not).

@rcomer
Copy link
Member

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
MemberAuthor

@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 theaxis 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 theaxis parameter and (ii) the registry can use classes with and without theaxis 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 currentScaleBase.__init__ is empty, so it does not matter. But not calling super init would break subclasses if we decide later that we needScaleBase.__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

Could this PR wait till after step 3?

timhoffm reacted with thumbs up emoji

@timhoffmtimhoffm marked this pull request as draftJanuary 5, 2025 22:19
@timhoffmtimhoffmforce-pushed thescale-axis branch 3 times, most recently from382aec0 to7c420d4CompareAugust 11, 2025 21:16
@timhoffmtimhoffm marked this pull request as ready for reviewAugust 11, 2025 21:16
# For backward compatibility, the built-in scales will keep the *axis* parameter
# in their constructors until matplotlib 3.15, i.e. as long as the *axis* parameter
# is still supported.
_scale_has_axis_parameter= {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need the separate variable, or can we get away with changing_scale_mapping todict[str, tuple[bool, ScaleBase]]?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

We could change_scale_mapping instead. I've considered that. In the end, I've chosen the separate variable, because this functionality is only needed during the deprecation period. It's simpler to delete the extra variable rather than rewrite the logic of_scale_mapping back to the previous behavior when the deprecation expires. But one can argument both ways. I don't have a strong opinion, but also don't think it really matters.

_scale_mapping[scale_class.name]=scale_class

# migration code to handle the *axis* parameter
has_axis_parameter="axis"ininspect.signature(scale_class).parameters
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that a subclass would name the parameter something other than "axis", in which case the better check here would be to see if there is a single positional only argument instead?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

In theory that would be possible. However, (i) it would be deeply confusing and I doubt a reaonsable subclass would do this; and (ii) it is possible that a subclass has defined two positional parametersSpecialScale(axis, foo), they might remove the axis and end up withSpecialScale(foo). We do not require and cannot enforce that subclasses do not have other positional parameters - therefore we cannot detect whetherSpecialScale(foo) wants to interpretfoo as axis or not. Therefore, I'd rather go by the name.

@dstansby
Copy link
Member

Looks good to me overall - I left a couple of suggestions to tighten language in the release note, and a question about the possiblity of other parameter names than "axis" inline.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@timhoffmtimhoffm merged commit2dd7d9f intomatplotlib:mainAug 14, 2025
38 of 39 checks passed
@timhoffmtimhoffm deleted the scale-axis branchAugust 14, 2025 10:36
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dstansbydstansbydstansby left review comments

@QuLogicQuLogicQuLogic approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

v3.11.0

Development

Successfully merging this pull request may close these issues.

4 participants

@timhoffm@rcomer@dstansby@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp