-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Move the call to Formatter.set_locs into Formatter.format_ticks. #13323
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a8c5557
to
d9361a5
Compare
The cleanup is reasonable. However, I'm stumbling a bit about the changed semantics. Can we get a more clear API? One can have two views on the formatter:
Do we actually need the state? If not one could do something like
These are just quick thoughts so far. They probably need a bit more consideration. |
We already pass the state - thats how ticks = [1, 1.125, 1.25, 1.5] are labeled with the same precision (1.00, 1.125, 1.250, 1.500). Its really makes little sense to not just format all the ticks at once. #10682 is a concrete example of where this is even more useful, and you can think of others. This PR just makes that logic less convoluted. |
(`~Formatter.set_locs` followed by repeated `~Formatter.__call__`) have been | ||
updated to use `~Formatter.format_ticks`. | ||
|
||
`~Formatter.format_tics` is intended to be overridden by `Formatter` subclasses |
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.
tics->ticks
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.
thanks, edited
The issue is that we secretly introduce state.
is explicit and However, after this PR is in, the code would be
and |
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.
Sorry, but I want to block until the API is more clear. I'm quite sure that there is a better way than having set_locs
separate from the formatting, but I have to look into this in more detail.
Are there contexts where you would set the locs, but not format the ticks? If not, I'm fine with |
As @jklymak mentioned, hoping that ticks can be formatted independently from one another is a long-lost battle; there are pretty good reasons why they're interdependent. |
d9361a5
to
1e54e65
Compare
I agree that the required tick format must depends on all ticks.
If we get this to work, I'm fine with the present PR, because then the hidden state magic will be gone. I want to look into this before dismissing my change request. |
All calls to Formatter.set_locs occur before a call to Formatter.format_ticks (or its predecessor, a list of calls to `Formatter.__call__`); it was basically a bandaid around the fact that all tick locations were needed to properly format an individual tick. Move the call to set_locs into format_ticks, with the intention to later deprecate set_locs. Convert a few more places to use format_ticks.
1e54e65
to
da55037
Compare
Just to clarify; are you saying you want the other PR (which doesn't exist right now, we're just discussing design) to be made before unblocking this one? |
Not necessarily. I want to be sure that it can be done and I would like to have it at least soon after this one. |
Actually, the easiest is probably just to lookup the tick locations on the axis instance rather than passing it in as argument (otherwise that's what you'll have to do at the call site anyways...), so something like
then slowly move through the different formatter classes to make them independent of set_locs. |
Hm, the dependence on the axis is another hidden state, and may actually cause incorrect behavior:
I wouldn't want to use the axis in the formatter unless really necessary. |
Well, again locators and formatters have known their axis since forever; I guess it would be reasonable to make set_axis raise an error if the axis has already been set (similarly to how artists cannot be reassigned from one axes to another). |
That would make sense. |
Well, except that ThetaLocator is of course doing funky stuff like
Probably can easily be worked around, but will take a bit longer than expected. |
Also, there's a bunch of formatters which actually do not care about the axis (NullFormatter, FixedFormatter, FuncFormatter, FormatStrFormatter, StrMethodFormatter) and these do get used across multiple axes in examples and tests; it also seems like gratuitious breakage to forbid their sharing :/ |
ping @timhoffm ... |
Target state:
Edit: Implementation detail:To maintain the major/minor aspect, formatter should have a flag if they are major or minor. The state should only contain the attibutes axis an is_major. Alternative B: Pass in the relevant Axis method Alternative C: Determine I tend to favor alternative C, it's a bit indirect but does not store redundant information. |
Well, I'm not sure what's the best way forward with 2) given my comment just above. |
Can we make that optional? The axis is passed in to the formatter, but the formatter can decide not to use axis information, in which case it can be attached to multiple axes. The check for multiple connections would be in the formatter. Add a class-level flag
|
OK, that's one possible design. Alternatively, we could make set_axis (.axis.setter) internally store a list of all axises this formatter has been assigned to, and error out when accessing the .axis attribute (if it has been set to multiple values), which would have the benefit of not requiring third-party formatters to change, and staying automatically correct for all formatter classes. However that'll likely be a separate PR (the problem of setting a formatter to multiple axises is not new). Marking this PR as release critical as it changes the semantics of the new format_ticks API and it would be much more of a pain to change this after it has been released. |
Unblocking. I think we're clear now, that the formatter should query the locs from the locator via the axis and not get them passed via set_locs
. In that sense, this PR is a reasonable step to internalize getting the locations.
All calls to Formatter.set_locs occur before a call to
Formatter.format_ticks (or its predecessor, a list of calls to
Formatter.__call__
); it was basically a bandaid around the fact thatall tick locations were needed to properly format an individual tick.
Move the call to set_locs into format_ticks, with the intention to later
deprecate set_locs.
Convert a few more places to use format_ticks.
Milestoning as 3.1 as format_ticks is a new API there so we can still change it before 3.1 is released...
PR Summary
PR Checklist