Make Scale axis parameter handling more flexible#31621
Make Scale axis parameter handling more flexible#31621QuLogic wants to merge 1 commit intomatplotlib:mainfrom
Conversation
QuLogic
added
the
Release critical
|
Also wondering if we should update the type hints to drop |
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
What previously worked was passing multiple parameters positionally LogitScale(axis, 'mask'). In rare cases, we one cannot just leave out the axis parameter. For FuncScale(functions) this works because functions has no default and thus the first bind attept fails, but for LogitScale('mask') this wrongly binds 'mask' to axis and keeps using the default non_positive='mask'.
That may be bearable as it only affects the new call pattern. Possibly we should advise users to always pass scale parameters by name. We already do this in scale_factory().
In theory we should _make_keyword_only to all scale parameters, but I cannot wrap my head around whether that would badly interact with _make_axis_parameter_optional right now.
Type stubs: I would only update them when we start deprecating using the axis parameter, which we intentionally do not do yet to smoothen the transition.
| FuncScale(None, (lambda x: x, lambda x: x)) | ||
| FuncScale(None, functions=(lambda x: x, lambda x: x)) | ||
| LogitScale(None) | ||
| LogitScale(None, 'clip') | ||
| LogitScale(None, nonpositive='clip') | ||
| LogitScale(None, use_overline=True) | ||
| AsinhScale(None) | ||
| AsinhScale(None, linear_width=2) | ||
| AsinhScale(None, base=3) | ||
| AsinhScale(None, subs=[2, 6]) | ||
| # Old signature with axis as keyword. | ||
| FuncScale(axis=None, functions=(lambda x: x, lambda x: x)) | ||
| LogitScale(axis=None) | ||
| LogitScale(axis=None, nonpositive='clip') | ||
| LogitScale(axis=None, use_overline=True) | ||
| AsinhScale(axis=None) | ||
| AsinhScale(axis=None, linear_width=2) | ||
| AsinhScale(axis=None, base=3) | ||
| AsinhScale(axis=None, subs=[2, 6]) |
There was a problem hiding this comment.
| FuncScale(None, (lambda x: x, lambda x: x)) | |
| FuncScale(None, functions=(lambda x: x, lambda x: x)) | |
| LogitScale(None) | |
| LogitScale(None, 'clip') | |
| LogitScale(None, nonpositive='clip') | |
| LogitScale(None, use_overline=True) | |
| AsinhScale(None) | |
| AsinhScale(None, linear_width=2) | |
| AsinhScale(None, base=3) | |
| AsinhScale(None, subs=[2, 6]) | |
| # Old signature with axis as keyword. | |
| FuncScale(axis=None, functions=(lambda x: x, lambda x: x)) | |
| LogitScale(axis=None) | |
| LogitScale(axis=None, nonpositive='clip') | |
| LogitScale(axis=None, use_overline=True) | |
| AsinhScale(axis=None) | |
| AsinhScale(axis=None, linear_width=2) | |
| AsinhScale(axis=None, base=3) | |
| AsinhScale(axis=None, subs=[2, 6]) | |
| # testing with None is sufficient as detection is purely based on the | |
| # signature structure, no type information is in involved | |
| axis=None | |
| FuncScale(axis, (lambda x: x, lambda x: x)) | |
| FuncScale(axis, functions=(lambda x: x, lambda x: x)) | |
| LogitScale(axis) | |
| LogitScale(axis, 'clip') | |
| LogitScale(axis, nonpositive='clip') | |
| LogitScale(axis, use_overline=True) | |
| AsinhScale(axis) | |
| AsinhScale(axis, linear_width=2) | |
| AsinhScale(axis, base=3) | |
| AsinhScale(axis, subs=[2, 6]) | |
| # Old signature with axis as keyword. | |
| FuncScale(axis=axis, functions=(lambda x: x, lambda x: x)) | |
| LogitScale(axis=axis) | |
| LogitScale(axis=axis, nonpositive='clip') | |
| LogitScale(axis=axis, use_overline=True) | |
| AsinhScale(axis=axis) | |
| AsinhScale(axis=axis, linear_width=2) | |
| AsinhScale(axis=axis, base=3) | |
| AsinhScale(axis=axis, subs=[2, 6]) |
There was a problem hiding this comment.
| called with or without the *axis* parameter. |
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
Actually, on a technical level, I can already approve. It's only some docs left.
Instead of checking the types ourselves, try to bind the old signature, and if possible, use it. Otherwise, try to use the new signature. This is similar to `_api.select_matching_signature` without needing to write a callable for each signature.
2 participants
PR summary
Instead of checking the types ourselves, try to bind the old signature, and if possible, use it. Otherwise, try to use the new signature. This is similar to
_api.select_matching_signaturewithout needing to write a callable for each signature.Note there are some invalid call signatures that might error in less than clear ways, but they already did so, and I did not attempt to make these clearer. (For example,
FuncScale(axis)will try to use theAxisasfunctionsand fail to unpack it as a tuple, but this was true with the original_make_axis_parameter_optional.)Closes #31590
AI Disclosure
None
PR checklist