10000 Simplify SecondaryAxis.set_color. by anntzer · Pull Request #14593 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Simplify SecondaryAxis.set_color. #14593

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 8, 2023
Merged

Simplify SecondaryAxis.set_color. #14593

merged 1 commit into from
Jul 8, 2023

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Jun 20, 2019 •
edited
Loading

The "orthogonal" axis is invisible, so we can just set the color of all
ticks, spines, and labels, regardless of orientation.

Note that the method is actually unused, untested, and could reasonably be lifted to the base class instead of only being provided for secondary axes, if we decide it's useful functionality (the new implementation doesn't use anything that's specific to secondary axes).

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

@tacaswell tacaswell added this to the v3.2.0 milestone Jun 22, 2019
@timhoffm
Copy link
Member
timhoffm commented Jul 4, 2019

I'm +/-0 on this PR. On the one hand, this is shorter. On the other hand, it feels awkward to set properties of objects that are not used.

I don't think, we should lift set_color() to _AxesBase. set_color() does only make sense for SecondaryAxis because it's sematically considered an Axis rather than an Axes.

The color of an Axis can well be assumed to be ticks, labels and spine of that axis. But the color of an Axes is ambiguous? Ticks, labels and spines of (both?) Axises? Or the facecolor?

@anntzer
Copy link
Contributor Author
anntzer commented Jul 4, 2019

On the other hand, it feels awkward to set properties of objects that are not used.

On the other hand, with the implementation here, it means that if someday someone comes up with e.g. a 3d version of secondary_axes, this implementation will directly work (cf a couple of other recent PRs by myself about mplot3d).

The color of an Axis can well be assumed to be ticks, labels and spine of that axis.

Then should we have Axis.set_color? (dunno, asking for opinions...)

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Aug 19, 2019
@QuLogic QuLogic added the status: needs comment/discussion needs consensus on next step label May 2, 2020
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 22, 2021
@jklymak jklymak marked this pull request as draft May 8, 2021 17:36
@anntzer anntzer marked this pull request as ready for review May 17, 2021 12:11
@jklymak jklymak marked this pull request as draft May 20, 2021 23:36
@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 18, 2021
@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022
@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 15, 2022
@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 Jun 21, 2023
@anntzer anntzer removed the status: inactive Marked by the “Stale” Github Action label Jun 26, 2023
@anntzer anntzer marked this pull 8000 request as ready for review June 26, 2023 08:05
@anntzer
Copy link
Contributor Author
anntzer commented Jun 26, 2023

The new version should be less controversial.

@oscargus
Copy link
Member

Seems like this is not tested. Maybe add a test for good measure? (And someone not breaking it at a later stage.)

@anntzer
Copy link
Contributor Author
anntzer commented Jul 4, 2023

sure, test added.
8000

@jklymak jklymak requested a review from oscargus July 6, 2023 16:37
@jklymak jklymak removed the status: needs comment/discussion needs consensus on next step label Jul 6, 2023
QuLogic
QuLogic reviewed Jul 7, 2023
View reviewed changes
@ksunden ksunden merged commit a861b8a into matplotlib:main Jul 8, 2023
@anntzer anntzer deleted the secondarycolor branch July 8, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@ksunden ksunden ksunden approved these changes

@timhoffm timhoffm timhoffm approved these changes

@oscargus oscargus Awaiting requested review from oscargus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.
7 participants
@anntzer @timhoffm @oscargus @QuLogic @ksunden @tacaswell @jklymak
0