Add location keyword argument to Colorbar by oscargus · Pull Request #23267 · matplotlib/matplotlib · GitHub

PR Summary

Is this an API change or a user change?

Will add an unrelated test for #23260 here as well. Added. However, they do not look great... The png looks OK, but locally I get in Spyder:

so there it seems like there is a bit of overlap that is not visible in the test image. For the svg there is a gap instead, although this seems to depend on the renderer...

Also, I do not understand why the test images are not just four squares as the pasted image...

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

New features are documented, with examples if plot related.
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).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

I also realized that there is a change here that may not really be wanted (or it may be). Earlier, passing e.g. orientation='horizontal' always gave ticklocation 'bottom'. Now, if passing orientation='horizontal' and a valid location, the ticklocation depends on if the location is 'top' or 'bottom'. Which makes sense and is consistent with what location does for make_axis and make_axis_gridspec.

This is awesome and I'm fairly sure should get a new feature/what's new write up.

Agree with the need for a what's-new.

this sounds like this kwarg only works with a passed in cax & if that's not the case than please also add an example for the not-cax case

The earlier version is documented, but is not used by colorbar (passed to either make_axes or make_axes_gridspec. I agree, however, that the current formulation is not ideal. Will try to modify it (now that I reminded myself of the differences).

I've not yet followed the details here, but I would have expected that this can build on top of _normalize_location_orientation. Am I wrong there?

_normalize_location_orientation also sets anchor, panchor and pad. I wasn't sure that one wanted those to be modified here? Also, here we need to keep the original value of location to keep track of if location and orientation was passed.

So while it could be used, it couldn't just be used to update the kwargs and it would do some additional stuff as well, which seemed redundant to me (but now there is redundant code instead...).

I'll think it through more carefully, but the rationale at the time was this.

So while it could be used, it couldn't just be used to update the kwargs and it would do some additional stuff as well, which seemed redundant to me (but now there is redundant code instead...).

I'll think it through more carefully, but the rationale at the time was this.

You could also introduce a lower-level function that is used here and inside of _normalize_location_orientation to avoid duplication - if that helps.

Got around to this and at least now it is checked and set in a single location.

Finally got back to this. The unrelated test is dropped.

Just a small documentation concern

I think this needs some description about how it interacts with orientation and ticklocation?

I've added a bit of text (copied from the top) and updated the docs for ticklocation. There should maybe be another way to describe how all these are related if only some are given.

Thanks @oscargus - definitely useful addition!

oscargus added the topic: color/colorbar label Jun 14, 2022

oscargus force-pushed the colorbarlocation branch from d976ac0 to f2cd4da Compare June 14, 2022 09:25

oscargus force-pushed the colorbarlocation branch 2 times, most recently from bdb43a3 to f44b0b4 Compare June 18, 2022 09:01

oscargus marked this pull request as ready for review June 18, 2022 09:05

jklymak reviewed Jun 20, 2022

View reviewed changes

Comment thread lib/matplotlib/tests/test_colorbar.py

oscargus force-pushed the colorbarlocation branch from f44b0b4 to 8695000 Compare June 20, 2022 10:24

story645 reviewed Jun 20, 2022

View reviewed changes

timhoffm reviewed Jun 21, 2022

View reviewed changes

oscargus commented Jun 22, 2022

View reviewed changes

Comment thread lib/matplotlib/colorbar.py

oscargus commented Jun 22, 2022

View reviewed changes

Comment thread lib/matplotlib/colorbar.py

oscargus marked this pull request as draft July 7, 2022 07:18

oscargus force-pushed the colorbarlocation branch from 8695000 to e0b1df1 Compare August 18, 2022 19:38

oscargus force-pushed the colorbarlocation branch 2 times, most recently from 08c2062 to 0442dd3 Compare August 18, 2022 19:52

oscargus added this to the v3.6.0 milestone Aug 18, 2022

oscargus marked this pull request as ready for review August 18, 2022 19:56

anntzer reviewed Aug 18, 2022

View reviewed changes

Comment thread lib/matplotlib/tests/test_colorbar.py Outdated

story645 reviewed Aug 18, 2022

View reviewed changes

Comment thread doc/users/next_whats_new/colorbar_has_location_argument.rst Outdated

Comment thread doc/users/next_whats_new/colorbar_has_location_argument.rst Outdated

Comment thread lib/matplotlib/colorbar.py Outdated

Comment thread lib/matplotlib/colorbar.py Outdated

tacaswell modified the milestones: v3.6.0, v3.7.0 Aug 19, 2022

oscargus force-pushed the colorbarlocation branch 3 times, most recently from 0515d39 to ee796c5 Compare November 2, 2022 13:31

anntzer approved these changes Nov 2, 2022

View reviewed changes

jklymak reviewed Nov 2, 2022

View reviewed changes

QuLogic modified the milestones: v3.7.0, v3.6.3 Nov 3, 2022

oscargus force-pushed the colorbarlocation branch from ee796c5 to e87cd5e Compare November 3, 2022 19:58

Add location keyword argument to Colorbar

8b4ad6c

oscargus force-pushed the colorbarlocation branch from e87cd5e to 8b4ad6c Compare November 6, 2022 10:08

jklymak approved these changes Nov 7, 2022

View reviewed changes

jklymak merged commit f975291 into matplotlib:main Nov 7, 2022

oscargus deleted the colorbarlocation branch November 7, 2022 04:38

stefmolin mentioned this pull request Nov 13, 2022

DOC: Fix colorbar what's new entry so it isn't cut off. #24440

Merged

1 task done

story645 mentioned this pull request Aug 25, 2023

[ENH]: alias colorbar location keyword to loc #26603

Open

+              The ``colorbar`` method now supports a ``location`` keyword argument
+              to more easily position the color bar. This is useful when providing
+              your own inset axes using the ``cax`` keyword argument and behaves


		filled : bool

		location : None or {'left', 'right', 'top', 'bottom'}

Add location keyword argument to Colorbar by oscargus · Pull Request #23267 · matplotlib/matplotlib · GitHub

Repository navigation

Add location keyword argument to Colorbar - #23267

Add location keyword argument to Colorbar#23267jklymak merged 1 commit intomatplotlib:mainmatplotlib/matplotlib:mainfrom oscargus:colorbarlocationoscargus/matplotlib:colorbarlocationCopy head branch name to clipboard

Conversation

oscargus commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

oscargus commented Jun 14, 2022

Uh oh!

story645 commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

story645 Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

oscargus Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

oscargus commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oscargus commented Jun 22, 2022

Uh oh!

timhoffm commented Jun 22, 2022

Uh oh!

oscargus commented Aug 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oscargus commented Nov 2, 2022

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

oscargus Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

jklymak commented Nov 7, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Add location keyword argument to Colorbar#23267
jklymak merged 1 commit into
matplotlib:mainfrom
oscargus:colorbarlocation

oscargus commented Jun 14, 2022 •
edited
Loading

story645 commented Jun 19, 2022 •
edited
Loading

oscargus commented Jun 22, 2022 •
edited
Loading