MNT: Registered 3rd party scales do not need an axis parameter anymore #29358
Conversation
70fc0eb to
bb30bef
Compare
|
Should the Custom Scale example also be updated at this point? Its |
|
I think the note here also needs updating matplotlib/lib/matplotlib/scale.py Lines 70 to 77 in ba32c7e |
|
@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 Specific note on:
I'd rather not go this way. The current |
|
Could this PR wait till after step 3? |
382aec0 to
7c420d4
Compare
| # 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.
Do we really need the separate variable, or can we get away with changing _scale_mapping to dict[str, tuple[bool, ScaleBase]]?
There was a problem hiding this comment.
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" in inspect.signature(scale_class).parameters |
There was a problem hiding this comment.
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.
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 parameters SpecialScale(axis, foo), they might remove the axis and end up with SpecialScale(foo). We do not require and cannot enforce that subclasses do not have other positional parameters - therefore we cannot detect whether SpecialScale(foo) wants to interpret foo as axis or not. Therefore, I'd rather go by the name.
|
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>
First step of #29349.