Repository navigation
MNT: Registered 3rd party scales do not need an axis parameter anymore - #29358
timhoffm merged 2 commits into
Conversation
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
I think the note here also needs updating matplotlib/lib/matplotlib/scale.py Lines 70 to 77 in ba32c7e |
Sorry, something went wrong.
|
@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:
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. |
Sorry, something went wrong.
|
Could this PR wait till after step 3? |
Sorry, something went wrong.
| # 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]]?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| _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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
First step of #29349.