Skip to content

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

Closed

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 15, 2019

PR Summary

See discussion starting at #13323 (comment), and #13438.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak
Copy link
Member

jklymak commented Feb 19, 2019

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?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 19, 2019

On the one hand, sharing (certain) locators and formatters across axis simply does not work. For example, after

import matplotlib.pyplot as plt
import matplotlib as mpl
fig, ax = plt.subplots()
loc = mpl.ticker.AutoLocator()
ax.xaxis.set_major_locator(loc)
ax.yaxis.set_major_locator(loc)
plt.show()

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 nf = NullFormatter(); ax.xaxis.set_major_formatter(nf); ax.yaxis.set_major_formatter(nf) works, and I would not be surprised that people are sharing FuncFormatters across axises:

fmt = FuncFormatter(lambda x, i: ...)
ax.xaxis.set_major_formatter(fmt)
ax.yaxis.set_major_formatter(fmt)

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...).

@jklymak
Copy link
Member

jklymak commented Feb 19, 2019

Perhaps just _log.info if a formatter/locator is shared between axis-es?

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.
@anntzer anntzer force-pushed the tickhelper-multiple-assignment branch from 8ebaae8 to ae58960 Compare February 19, 2019 15:35
@anntzer
Copy link
Contributor Author

anntzer commented Feb 19, 2019

I agree that this may be quite overkill and just logging may be enough. Perhaps @timhoffm wants to chime in?

@timhoffm
Copy link
Member

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?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 21, 2019

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.
Also I don't really like the idea of introducing a new public API just for that purpose.

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.

@timhoffm
Copy link
Member

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.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 21, 2019

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.
The advantage of logging at the info level is that it is not on by default, and we can easily word it as e.g. "the ticker foo is being assigned to a second axis, this may cause problems; consider instantiating a new ticker" etc. which (similarly to the strings/categoricals) issue can help if someone gets confused and decides to check the log output, but otherwise won't annoy users who just reuse nullformatters.

@timhoffm
Copy link
Member

timhoffm commented Feb 21, 2019

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).

can easily go outdated.

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.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 21, 2019

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).

@jklymak
Copy link
Member

jklymak commented Feb 21, 2019

I think so as well. I understand the intellectual concern, but I'm not sure the practical concern is worth so much complicated machinery.

@timhoffm
Copy link
Member

No complaint is not a strong argument in this case.

  1. Probably not too many people are reusing locators anyway. This makes the case for not adding complicated code. But OTOH would allow to simply deprecate reuse without affacting many users.
  2. IMHO most of the time an invalid reuse is taking place people will not notice or don‘t care enough to dig into why the ticks are awkward.

The simple class level variable
#13323 (comment) would bring us 90% the way, by having clean messages/behavior for all the builtin Locators. I don‘t care too much on the message for user defined locators, which can be the vague message proposed above.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 21, 2019

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 get_major_formatter() (and friends) create a temporary FormatterWithAxisAttached object that wraps the Formatter and attaches the Axis info to it.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 21, 2019

See #13482 for an alternative (simpler?) design.

@github-actions
Copy link

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.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 31, 2023
@anntzer
Copy link
Contributor Author

anntzer commented May 31, 2023

I now think the way forward, if we want to fix this, is to move towards a stateless ticker API (where you always call locator.locate(axis)/formatter.format_ticks(axis, ticks)).

@anntzer anntzer closed this May 31, 2023
@anntzer anntzer deleted the tickhelper-multiple-assignment branch June 1, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants