Skip to content

Maintain color of tick labels #22856

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
wants to merge 1 commit into from
Closed

Conversation

hhalwan
Copy link

@hhalwan hhalwan commented Apr 17, 2022

PR Summary

Fixes this issue. spine.set_position currently resets the axis's ticks, thereby resetting the color of the tick labels. I made it so the color is saved before and restored after reset_ticks is called. Also, reset_ticks is now only called when the position is not equal to ('outward', 0.0). This is to prevent resetting and applying color during the subplot initialization (during which set_position is called four times), because applying color before the initialization is done seems to break things and resetting the ticks isn't necessary then.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

… to spine.set_position

set_position currently resets the color of tick labels after a call to reset_ticks. This commit saves the color and restores the color to the tick labels after reset is called. Also, reset_ticks is now only called when the position is not 'outward', 0.0. This is to prevent resetting and applying color during the subplot initialization. Also added a test case to check this.
@timhoffm
Copy link
Member

Thanks for the PR! Good work, in particular the consideration if the initialization code path! However, I think this needs more careful consideration. The current tick mechanism it’s somewhat complicated.

First we have to distinguish between general default tick properties and those of individual ticks. If you set the color of individual ticks, future generated ticks are not affected. I’m away from my dev environment, so can’t check, but I guess the issue does not arise if you use ax.tick_params() in your example. This is the recommended way if you want to change a property of all ticks.

This should solve your use case, but of course not the case if someone wants to set the color of the first tick only. Conceptually, I’m unclear why set_position() needs to reset the ticks at all. I would have thought that moving the spine should not affect the ticks at all. Anyway, reapplying only the car solves the issue only for that property. However all other properties like font size and rotation will suffer from the same issue as well. So if you want to fix this we need a more general approach.

@timhoffm
Copy link
Member

Also to be checked: #20805 should maintain tick properties after reset, but apparently doesn’t?!?

@oscargus oscargus added the status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. label Apr 18, 2022
@jklymak
Copy link
Member

jklymak commented Apr 18, 2022

I’m unclear why set_position() needs to reset the ticks at all.

So this is due to the lazy instantiation of the ticks. If you change the color of each tick label, you instantiate the ticks, including their x/y position. If you subsequently move the spine, the ticks have already been instantiated, and the ticks don't come along with the spine unless you reset them.

Setting tick labels and their properties manually will always get invalidated if there is another change to the axis afterwards. What is more mysterious is that this sometimes works, because we instantiate a list of ticks that is empty before the axis is fully setup. So

fig, ax = plt.subplots()
ax.plot([1, 2, 3])
for lab in ax.get_yticklabels():
    lab.set_color("r")
ax.set_ylim([-1, 4])
plt.show()

will have red labels, because we set the empty labels to red, and then reused them.

So this is a real consistency problem, at least to my taste. I'd prefer the above and the original issue to error out, or evaluate the ticks properly, and populate with "real" ticks. If the user wants to change those, great. If the view changes, and the ticks are invalid, then their changes should be lost. I think all this would require is a state on the axis that says if the tick list is real or not (if we are unwilling to instantiate when needed for some reason), and then either error if there is no real list of ticks either create one, or Raise.

@timhoffm
Copy link
Member

timhoffm commented Apr 18, 2022

Yes, operating on single tick instances is almost never the right way to go. Ticks can change thoughout the lifetime of a figure. We need to make the gobal tick properties more a first class concept (we internally have that though _major_tick_kw). On the API (and maybe internal) side we should move away from single ticks. I have a pre-draft branch, that tries to wrap all that logic behind a fascade.

@hhalwan Thanks for looking into this. However I'm afraid you've poked quite a rabbit hole here and the correct solution has to go much deeper (Apart from the user side, which should use tick_params(labelcolor=...) and not iterate though the existing ticks).

@oscargus oscargus removed the status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. label May 29, 2022
@jklymak
Copy link
Member

jklymak commented Jun 30, 2022

@hhalwan I think this is probably not going to go in like this. But feel free to advocate for its inclusion if you strongly feel this would help users. The fundamental is that our tick interface is a) expensive, b) very flexible and c) very complicated.

@jklymak jklymak added the status: needs comment/discussion needs consensus on next step label Jun 30, 2022
@timhoffm
Copy link
Member

Semi-related: #23372 - Use tick_params() instead of iterating over tick labels for setting tick properties

@jklymak
Copy link
Member

jklymak commented Nov 24, 2022

@hhalwan Sorry this didn't move forward, but as @timhoffm notes above this is a very complicated part of our code base. I'm going to close, but if you still feel strongly this can work, feel free to request a re-open. We very much hope to hear from you again!

@jklymak jklymak closed this Nov 24, 2022
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.

[Bug]: set_position function of spine will affect the color of ticklabels
4 participants