Skip to content

Update _base.py #16141

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

Merged
merged 6 commits into from
Jan 9, 2020
Merged

Update _base.py #16141

merged 6 commits into from
Jan 9, 2020

Conversation

pgierz
Copy link
Contributor

@pgierz pgierz commented Jan 7, 2020

Adds an additional note to clarify get_xticklabels

PR Summary

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

Adds an additional note to clarify get_xticklabels
jklymak
jklymak previously requested changes Jan 7, 2020
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking on comment below, but feel free to clear if I'm misunderstanding something. I think this should be turned into an issue that should be fixed rather than something that should be worked around.

Notes
-----
Please note that the tick label list is not populated until ``plt.draw()`` has been called.
See also: https://stackoverflow.com/questions/32700935/get-xticklabels-contains-empty-text-instances
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont' think we usually call out to outside resources (they are fragile). If there is something pertinent in that post please include here.

I think this is actually a bug, and the lazy tick instantiation should be triggered by get_xticklabels. I think before we did this lazily, this used to work? ping @anntzer @efiring

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raised an issue for this (#16140). I'd be happy to expand on that and provide a minimal example if it helps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we never did (at least AFAIK) and the current behavior was always present, but I agree we could consider changing it.

I also agree the SO link could be skipped.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot remember of this ever being different than it is now. It's pretty inherent to the fact that the locator first needs to determine the ticks and the formatter finding their labels. This can only be done when drawing the axes - else, it might still change afterwards.
For that reason I would actually argue for not changing the behaviour; because as it stands, we can at least be sure that get_xticklabels() will never return anything that is only seemingly correct.

Adding a note in the docs seems useful though. (No SO link needed IMO. Instead a reference to pyplot.draw and canvas.draw)

Also, I would dare to claim that in 90% of the cases where people might want to use .get_{x,y}ticklabels, there would be a better solution. (E.g. in the linked SO post, they want to rotate labels, for which one does not actually need to know the ticklabels.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then I think this leads to the question: what is the point of this function at all? Presumably its to get the tick labels and change them (i.e. add a dollar sign or a happy face to all of them). If the user wants to do this, and avoid writing their own formatter, then I'd argue they shouldn't need to call an explicit draw in order to do so. Or, if we do need to call a draw (because it's impossible to know the limits before a draw) then maybe this function should call the draw. But I think the function is fundamentally broken if we have to explicitly tell users to call draw to get it to work.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least before 3.0 ax.get_position() used to give wrong positions unless you drew the canvas. Not sure what exactly changed in between. But to construct an example where this is still the case, we can use an inset axes:

import matplotlib.pyplot as plt
import numpy as np

fig, ax = plt.subplots()
ax.imshow(np.random.rand(10,4))
axins = ax.inset_axes([0.0, 0.4, 0.7, 0.5])

print(axins.get_xticklabels())
# prints "wrong" labels: <a list of 6 Text xticklabel objects>
print(axins.get_position())
# prints "wrong" position: Bbox(x0=0.125, y0=0.418, x1=0.6675, y1=0.803)

fig.canvas.draw()

print(axins.get_xticklabels())
# prints "correct" labels: <a list of 3 Text xticklabel objects>
print(axins.get_position())
# prints "correct" position: Bbox(x0=0.397, y0=0.418, x1=0.5587, y1=0.803)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but thats a pretty specialized example using set_axes_locator. I agree that any axes that use `set_axes_locator (including everything in axes_grid) will not return a sensible position until the locator function is called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... though come to think of it, its possible the axes_locator function could be called if get_position() is called.

Copy link
Contributor Author

@pgierz pgierz Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi everyone,

I've updated the documentation improvement suggestion:

  • removed the link to SO
  • added references to pyplot.draw and canvas.draw

I guess the same problem will also apply to the yticklabels method? A colleague (@christian-stepanek) and I had the idea that if the function should not be changed, maybe it would at least be helpful to add a warning message so the user is informed that the tick label list has not yet been populated. Would something like this be feasible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, set_yticklabels should get the same text. 👍

I'm -0.5 on adding a warning. We tend to be cautious to issue warnings when we are not sure something is going wrong. It's annoying to issue warnings for valid use cases. There are (few) cases when one might want to have the tick Text instances and the actual string values are irrelevant.

@tacaswell tacaswell added this to the v3.3.0 milestone Jan 7, 2020

Notes
-----
Please note that the tick label list is not populated until a ``draw``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe more precisely:.. the tick label strings are not not...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@jklymak jklymak dismissed their stale review January 8, 2020 18:29

Seems fine now +/- the wording...

Please note that the tick label strings are not populated until a ``draw``
method has been called.

See also: `~pyplot.draw` and `~canvas.draw`.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to fix these references for CI to be happy...

pgierz and others added 3 commits January 9, 2020 07:11
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Removes whitespace and adds a similar note for `get_yticklabels`
Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pgierz ! This looks great 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants