-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Prevent reuse of certain Locators and Formatters over multiple Axises. #13439
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
89694ed
to
8ebaae8
Compare
I'm not following this PR, and maybe its great, but... why are we doing this huge song and dance? I thought the point was to make this all simpler, not add new setters getters and helper classes? Is it really gratuitous breakage to say that we won't allow sharing Locators and Formatters? What context could these possibly need to be shared across axes? Do people really do that, and if so, why? |
On the one hand, sharing (certain) locators and formatters across axis simply does not work. For example, after
if you pan the x-axis you see that it has no ticks beyond [0, 1] because the ticker gets its bounds from the y limits. On the other hand, this is only true for certain locators and formatters. In particular, NullFormatter and FuncFormatter are clearly stateless (they never access the axis info, just format the ticks they are given); we do have tests that check that
seems natural enough to write. Of course we could just says that the "don't share tickers across axises if they lookup the axis info" limitation just needs to be documented rather than having piles of code to check that (although it may not be obvious to the user which ones can be shared...). |
Perhaps just This is a lot of code to basically help the user not trip over themselves. |
The call to _wrap_locator_formatter was deleted from ThetaAxis.gca() because it is redundant with the call in ThetaAxis._set_scale (which gca() calls), and would cause double wrapping with _AxisWrapper, causing issues when checking for axis equality.
8ebaae8
to
ae58960
Compare
I agree that this may be quite overkill and just logging may be enough. Perhaps @timhoffm wants to chime in? |
Sorry not much bandwidth at the moment. So, I haven't checked the implementation here in detail. Generally, I'd prefer that formatters don't allow multiple assignments if they use the axis info. It's an inconsistent state if a formatter is assigned to multiple axes but obtains the locations only from one. No good can come from that. The plot will almost always be wrong and we just shouldn't allow people to shoot themselves in the foot. Don't set up a warning sign. Take away the gun 😄. Isn't the solution proposed in #13323 (comment) working and simpler? |
Then you need to make the default value for use_axis_context False in order to not break third-party tickers that don't use the axis info, but then they don't really benefit from the check in the more common(?) case where they do. Again I think logging multiple-assignment at info level may be enough; after all I don't remember seeing anyone complain about this issue and being confused about it. |
You're right about the third-party tickers. Not sure if I would worry too much about them though. It's far more common to use a shipped locator. And if people define their own locator, they usually know what they are doing and it's probably in a local scope of their tooling. So not giving them the benefit of this PR would be a not too bad trade off. Also for logging, we would need the same logic. We don't want to issue unnecessary and thus misleading warnings for e.g. FuncFormatter. The other alternative would be to deprecate reuse of locators completely. It's not too difficult to instantiate one locator per axis. The advantage is that the user has just a simple rule. |
Even if we don't worry about third-party locators, that's still a field that we need to be careful to set properly on our own tickers, and can easily go outdated. |
Not quite convinced. „this may cause problems; consider instantiating a new ticker“ is quite vague. As a user, you don‘t know if and how your use case is accepted. Users of NullFormatters may get overly cautious and instantiate separately, just to be safe. Others may not see or ignore the waning and end up with bad ticks (possibly even without realizing it).
I would be willing to take that risk. We will add new locators only very occasionally, and once setup the existing locators should not get outdated. |
Again, I feel like we're just inventing a problem out of nowhere here. This limitation of tickers has been around for a long time, and no one has ever complained about it (to the best of my knowledge). |
I think so as well. I understand the intellectual concern, but I'm not sure the practical concern is worth so much complicated machinery. |
No complaint is not a strong argument in this case.
The simple class level variable |
I really don't think we should deprecate reusing (for example) FuncFormatters. That's working just fine. For example right now the tick-formatters.py example demonstrates (in particular) using FuncFormatter as a decorator, and that's slightly annoying to rewrite if you need to get hold on the function first to create multiple formatter instances. Even if we move to a class-level variable defining whether formatters can be reused, we need escape routes to allow the reassignment of any formatter (see the figure.py and polar.py changes in this PR), which is quite ugly. If we really want to "fix" this, I think a better solution may be to make ticker not store the axis info and make |
See #13482 for an alternative (simpler?) design. |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
I now think the way forward, if we want to fix this, is to move towards a stateless ticker API (where you always call |
PR Summary
See discussion starting at #13323 (comment), and #13438.
PR Checklist