Deprecate setting Axis.major.locator to non-Locator; idem for Formatters #13851
Conversation
See changelog. This is so that we can actually add new APIs to the base class (e.g. format_ticks; tick_deconflict (however that one ends up being named) and actually benefit from it in the rest of the codebase without worrying that third-party locators/formatters do not inherit from the base class (note that inheriting from the base class does not preclude third-party locators/formatters from completely redefining all methods, so this does not limit them).
| Setting ``Axis.major.locator``, ``Axis.minor.locator``, ``Axis.major.formatter`` | ||
| or ``Axis.minor.formatter`` to an object that is not a subclass of `Locator` or | ||
| `Formatter` (respectively) is deprecated. Note that these attributes should | ||
| usually be set using `Axis.set_major_locator`, `Axis.set_minor_locator`, etc. |
There was a problem hiding this comment.
Should they be set at all directly on the ticker? I don't think so at least for locators, because Axis.set_major_locator ensures that the formatter knows the locator, which is not guaranteed when setting Axis.major.locator.
So maybe add _set_locator() and _set_formatter() for the internal API. And deprecate property setter for all inputs.
There was a problem hiding this comment.
Well, you get to argue about https://github.com/matplotlib/matplotlib/pull/13826/files#diff-52127080e2783464f529df684d82d476R1319 with @tacaswell then :)
(Note that we could move the call to _set_locator in locator.setter, though.)
There was a problem hiding this comment.
I don't see the point. The linked diff does not assign to axis.major.locator or axis.major.formatter?
There was a problem hiding this comment.
It does getattr(self.minor.locator, "remove_overlaps", True) instead of the simpler self.minor.locator.remove_overlaps (note that remove_overlaps is in the base Locator class) specifically to handle the case of a locator not inheriting from Locator.
There was a problem hiding this comment.
Ok, I see that we jump through some hoops to support non-Locator instances as locator.
But _set_locator() would be living in TickHelper, which hopefully nobody has replaced. AFAICS non-Locator objects would still work.
There was a problem hiding this comment.
Sorry, I don't get what you're saying about TickHelper. Did you mean Ticker (which is not the same)?
| @@ -640,8 +640,36 @@ class Ticker(object): | |||
| formatter : `matplotlib.ticker.Formatter` subclass | |||
| Determines the format of the tick labels. | |||
| """ | |||
| locator = None | |||
| formatter = None | |||
There was a problem hiding this comment.
I hope that removing these (public) class attributes isn't going to trip anyone up. Maybe that would involve such an obscure use case that it won't happen.
There was a problem hiding this comment.
We did just have that issue, but in this case I suspect it will be ok.
These lines go back to d78e6c4 and I suspect it was done done to make sure that Ticker sub-classes always had the attributes. Putting something into these attributes at the class level does not really make sense.
See changelog.
This is so that we can actually add new APIs to the base class (e.g.
format_ticks; tick_deconflict (however that one ends up being named)
and actually benefit from it in the rest of the codebase without
worrying that third-party locators/formatters do not inherit from the
base class (note that inheriting from the base class does not preclude
third-party locators/formatters from completely redefining all methods,
so this does not limit them).
(e.g. https://github.com/matplotlib/matplotlib/pull/13826/files#diff-52127080e2783464f529df684d82d476R1319)
PR Summary
PR Checklist