Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
70fc0eb tobb30befComparercomer commentedJan 5, 2025
Should theCustom Scale example also be updated at this point? Its |
rcomer commentedJan 5, 2025
I think the note here also needs updating matplotlib/lib/matplotlib/scale.py Lines 70 to 77 inba32c7e
|
timhoffm commentedJan 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 theaxis parameter in Specific note on:
I'd rather not go this way. The current |
rcomer commentedJan 5, 2025
Could this PR wait till after step 3? |
382aec0 to7c420d4CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| # 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= { |
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.
Do we really need the separate variable, or can we get away with changing_scale_mapping todict[str, tuple[bool, ScaleBase]]?
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| _scale_mapping[scale_class.name]=scale_class | ||
| # migration code to handle the *axis* parameter | ||
| has_axis_parameter="axis"ininspect.signature(scale_class).parameters |
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.
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?
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
dstansby commentedAug 12, 2025
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. |
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
2dd7d9f intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
First step of#29349.