~~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 (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

oscargus added the topic: color/colorbar label

Jun 14, 2022

oscargus force-pushed thecolorbarlocation branch fromd976ac0 tof2cd4daCompare

June 14, 2022 09:25

Copy link

MemberAuthor

oscargus commentedJun 14, 2022

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 passingorientation='horizontal' and a validlocation, the ticklocation depends on if the location is'top' or'bottom'. Which makes sense and is consistent with what location does formake_axis andmake_axis_gridspec.

oscargus force-pushed thecolorbarlocation branch 2 times, most recently frombdb43a3 tof44b0b4Compare

June 18, 2022 09:01

oscargus marked this pull request as ready for review

June 18, 2022 09:05

Copy link

Member

story645 commentedJun 19, 2022•
edited
Loading

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

jklymak reviewed

Jun 20, 2022

View reviewed changes

Copy link

Member

jklymak left a comment

Choose a reason for hiding this comment

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

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

lib/matplotlib/tests/test_colorbar.pyShow resolvedHide resolved

oscargus force-pushed thecolorbarlocation branch fromf44b0b4 to8695000Compare

June 20, 2022 10:24

story645 reviewed

Jun 20, 2022

View reviewed changes

doc/users/next_whats_new/colorbar_has_location_argument.rst Outdated


		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

Copy link

Member

story645Jun 20, 2022

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

oscargusJun 20, 2022

Choose a reason for hiding this comment

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

The earlier version is documented, but is not used bycolorbar (passed to eithermake_axes ormake_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).

timhoffm reviewed

Jun 21, 2022

View reviewed changes

Copy link

Member

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'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?

Copy link

MemberAuthor

oscargus commentedJun 22, 2022•
edited
Loading

_normalize_location_orientation also setsanchor,panchor andpad. I wasn't sure that one wanted those to be modified here? Also, here we need to keep the original value oflocation to keep track of iflocation andorientation 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...).

oscargus commented

Jun 22, 2022

View reviewed changes

lib/matplotlib/colorbar.pyShow resolvedHide resolved

oscargus commented

Jun 22, 2022

View reviewed changes

lib/matplotlib/colorbar.pyShow resolvedHide resolved

Copy link

MemberAuthor

oscargus commentedJun 22, 2022

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

Copy link

Member

timhoffm commentedJun 22, 2022

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.

oscargus marked this pull request as draft

July 7, 2022 07:18

oscargus force-pushed thecolorbarlocation branch from8695000 toe0b1df1Compare

August 18, 2022 19:38

Copy link

MemberAuthor

oscargus commentedAug 18, 2022

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

oscargus force-pushed thecolorbarlocation branch 2 times, most recently from08c2062 to0442dd3Compare

August 18, 2022 19:52

oscargus added this to thev3.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

lib/matplotlib/tests/test_colorbar.py OutdatedShow resolvedHide resolved

story645 reviewed

Aug 18, 2022

View reviewed changes

doc/users/next_whats_new/colorbar_has_location_argument.rst OutdatedShow resolvedHide resolved

lib/matplotlib/colorbar.py OutdatedShow resolvedHide resolved

tacaswell modified the milestones:v3.6.0,v3.7.0

Aug 19, 2022

oscargus force-pushed thecolorbarlocation branch 3 times, most recently from0515d39 toee796c5Compare

November 2, 2022 13:31

Copy link

MemberAuthor

oscargus commentedNov 2, 2022

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

anntzer approved these changes

Nov 2, 2022

View reviewed changes

jklymak reviewed

Nov 2, 2022

View reviewed changes

Copy link

Member

jklymak left a comment

Choose a reason for hiding this comment

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

Just a small documentation concern

lib/matplotlib/colorbar.py Outdated


		ticklocation : {'auto', 'left', 'right', 'top', 'bottom'}

		drawedges : bool

		filled : bool

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

Copy link

Member

jklymakNov 2, 2022

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

oscargusNov 3, 2022

Choose a reason for hiding this comment

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

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.