Skip to content

ENH: add long_axis property to colorbar - #27167

Merged
jklymak merged 4 commits into
matplotlib:mainfrom
jklymak:enh-expose-long-axis
Oct 28, 2024
Merged

ENH: add long_axis property to colorbar#27167
jklymak merged 4 commits into
matplotlib:mainfrom
jklymak:enh-expose-long-axis

Conversation

@jklymak

@jklymak jklymak commented Oct 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread lib/matplotlib/colorbar.py Outdated

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

@anntzer anntzer Oct 23, 2023

Copy link
Copy Markdown
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

@anntzer anntzer left a comment

Copy link
Copy Markdown
Contributor

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

jklymak commented Oct 24, 2023

Copy link
Copy Markdown
Member Author

I think I replaced all the _long_axis calls.

@timhoffm

timhoffm commented Oct 24, 2023

Copy link
Copy Markdown
Member

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

story645
story645 reviewed Oct 24, 2023
View reviewed changes

@story645 story645 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

@story645 story645 left a comment

Copy link
Copy Markdown
Member

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

timhoffm commented Sep 20, 2024

Copy link
Copy Markdown
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

@timhoffm timhoffm left a comment

Copy link
Copy Markdown
Member

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

timhoffm commented Oct 28, 2024

Copy link
Copy Markdown
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

jklymak commented Oct 28, 2024

Copy link
Copy Markdown
Member Author

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
@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
This was referenced Oct 27, 2025
Closed
Closed
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