Skip to content

ENH: add long_axis property to colorbar #27167

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 4 commits into from
Oct 28, 2024
Merged

ENH: add long_axis property to colorbar #27167

merged 4 commits into from
Oct 28, 2024

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Oct 22, 2023 •
edited
Loading

PR summary

As discussed in #26896 exposing a long_axis property for colorbar.

PR checklist

@jklymak jklymak requested a review from anntzer October 22, 2023 23:39
anntzer
anntzer reviewed Oct 23, 2023
View reviewed changes
lib/matplotlib/colorbar.py Outdated
@@ -433,6 +433,11 @@ def __init__(self, ax, mappable=None, *, cmap=None,
self._extend_cid2 = self.ax.callbacks.connect(
"ylim_changed", self._do_extends)

@property
def long_axis(self):
"""Axis that has decorations (ticks etc) on it."""
Copy link
Contributor

Choose a reason for hiding this comment

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

comma before etc.

anntzer
anntzer approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Looks fine to me; we could consider replacing all internal uses of _long_axis() by the public long_axis but there's no urgency to do that.

@jklymak
Copy link
Member Author

jklymak commented Oct 24, 2023

I think I replaced all the _long_axis calls.

@timhoffm
Copy link
Member

Can we please still have a short discussion on naming? See #26896 (comment)

story645
story645 reviewed Oct 24, 2023
View reviewed changes
Copy link
Member

@story645 story645 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Blocking to prevent accidental merge until this is discussed per @timhoffm's comments

I don't love value or color cause those are the fill values of the colorbar (texture could be added instead of color) and not exactly the axis of the bar frame.

story645
story645 previously requested changes Oct 24, 2023
View reviewed changes
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

Forgot to actually select request changes 🤦‍♀️

@timhoffm
Copy link
Member

Let's move this forward. I still favor data_axis because it captures the semantic information aspect rather than just geometry. I'd be happy if you would see a value in switching to that.

I just started to like data_axis. While data is a very generic term and should usually be avoided, maybe we actually want that generic aspect here. We don't need to focus on the exact type of data (one can think label, color, numeric value as one likes). data generically captures the semantic information aspect, contrasting from the other non-data axis.

But if not, let's not hold this up longer.

@timhoffm timhoffm dismissed story645’s stale review September 20, 2024 13:32

Naming is being discussed.

timhoffm
timhoffm approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I'd favor to switch to data_axis. But if you don't follow that argument long_axis is still ok.

@jklymak please decide yourself and merge afterwards.

@timhoffm
Copy link
Member

@jklymak should we drop this still into 3.10?

@jklymak jklymak force-pushed the enh-expose-long-axis branch from de0ce33 to fecdb54 Compare October 28, 2024 15:47
@jklymak
Copy link
Member Author

jklymak commented Oct 28, 2024

I'm going to go ahead and merge. Thanks for the ping. I still prefer "long axis" ;-)

@jklymak jklymak merged commit 36daea4 into matplotlib:main Oct 28, 2024
43 checks passed
@jklymak jklymak deleted the enh-expose-long-axis branch October 28, 2024 17:21
@QuLogic QuLogic added this to the v3.10.0 milestone Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer approved these changes

@timhoffm timhoffm timhoffm approved these changes

@story645 story645 story645 left review comments

Assignees
No one assigned
Labels
topic: color/color & colormaps topic: color/colorbar
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jklymak @timhoffm @story645 @anntzer @QuLogic