This corrects a few mistakes in positioning PRs that were preventing locatable axes from working correctly. It also makes a couple of kwargs explicit rather than positional when we call them.

PR Checklist

Has Pytest style unit tests
Code isFlake 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

Copy link

Member

ImportanceOfBeingErnest commentedOct 1, 2018

Maybe this is a good place to add a test forthe failing usecase?

jklymak added status: work in progress and removed status: work in progress labels

Oct 1, 2018

Copy link

MemberAuthor

jklymak commentedOct 2, 2018

New test fails master, passes this PR and was the root of the problem....

jklymak added this to thev3.0.x milestone

Oct 2, 2018

jklymak mentioned this pull request

Oct 2, 2018

ImageGrid bug when using inline backend#12370

Closed

jklymak force-pushed thefix-correct-get-position-errors branch from59a994b toa5e6667Compare

October 2, 2018 15:44

jklymak added Release critical

For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

status: needs review labels

Oct 2, 2018

Copy link

Member

timhoffm commentedOct 3, 2018•
edited
Loading

Please rebase.#12366 is in and should fix the failing CI.

timhoffm added the status: needs rebase label

Oct 3, 2018

jklymak force-pushed thefix-correct-get-position-errors branch froma5e6667 to0ce489cCompare

October 3, 2018 15:26

jklymak removed the status: needs rebase label

Oct 3, 2018

Copy link

MemberAuthor

jklymak commentedOct 3, 2018

...rebased

Copy link

Member

timhoffm commentedOct 3, 2018

flake8 says

./lib/mpl_toolkits/tests/test_axes_grid1.py:399:57: W291 trailing whitespace

jklymak force-pushed thefix-correct-get-position-errors branch from0ce489c to1da1c35Compare

October 3, 2018 17:49

Copy link

MemberAuthor

jklymak commentedOct 3, 2018

Fixed flake8. I'd actually be in favour of adding an Exception for the trailing whitespace error.

timhoffm approved these changes

Oct 3, 2018

View reviewed changes

WeatherGod reviewed

Oct 4, 2018

View reviewed changes

Copy link

Member

WeatherGod left a comment

Choose a reason for hiding this comment

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

everything else looks good, just have a comment about a code alignment.

lib/matplotlib/figure.py Outdated

		@@ -2298,7 +2298,8 @@ def get_tightbbox(self, renderer, bbox_extra_artists=None):

		for ax in self.axes:
		if ax.get_visible():
		bb.append(ax.get_tightbbox(renderer, bbox_extra_artists))
		bb.append(ax.get_tightbbox(renderer,
		bbox_extra_artists=bbox_extra_artists))

Copy link

Member

WeatherGodOct 4, 2018

Choose a reason for hiding this comment

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

The alignment of this keyword argument makes me think it is for the append, not the get_tightbbox(), but even that looks wonky. Which PEP8 alignment rule is this?

Copy link

MemberAuthor

jklymakOct 4, 2018

Choose a reason for hiding this comment

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

I just did two-tab (8-space) indentation - I don't think it'll fit on one line, though I can check

Copy link

Member

QuLogicOct 4, 2018

Choose a reason for hiding this comment

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

It will not, nor will it work with aligning to the correct opening parenthesis, but you can do this:

bb.append(ax.get_tightbbox(renderer,bbox_extra_artists=bbox_extra_artists))

Copy link

MemberAuthor

jklymakOct 4, 2018

Choose a reason for hiding this comment

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

Ummm, OK, butreally the 8-space indent is allowed by PEP8, and in this case I think looks best:

https://www.python.org/dev/peps/pep-0008/?#indentation

Copy link

Member

QuLogicOct 4, 2018

Choose a reason for hiding this comment

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

I'm meant "will not fit", not anything about whether it's in PEP8.

Copy link

Member

QuLogicOct 4, 2018

Choose a reason for hiding this comment

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

That PEP8 link uses 4-space indent for continuation lines; it's only if there's ambiguity that double indent is used (but keep in mind there are no nested function calls in those examples.)

Copy link

Member

timhoffmOct 4, 2018

Choose a reason for hiding this comment

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

I think a list comprehension would be more readable anyway.

Copy link

MemberAuthor

jklymakOct 4, 2018

Choose a reason for hiding this comment

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

OK, list comprehension is bearable.

Copy link

Contributor

anntzerOct 4, 2018

Choose a reason for hiding this comment

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

Uh, I think (hope?)@timhoffm meant something like

        bbox.extend(            ax.get_tightbbox(renderer, bbox_extra_artists=bbox_extra_artists)            for ax in self.axes if ax.get_visible())

Copy link

MemberAuthor

jklymakOct 4, 2018

Choose a reason for hiding this comment

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

Fine

lib/mpl_toolkits/axes_grid1/parasite_axes.py Outdated

		for ax in self.parasites]
		bbs.append(super().get_tightbbox(renderer, call_axes_locator))
		bbs.append(super().get_tightbbox(renderer,
		call_axes_locator=call_axes_locator))

Copy link

Member

WeatherGodOct 4, 2018

Choose a reason for hiding this comment

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

similar issue here, the alignment of the keyword argument is wonky.

Copy link

MemberAuthor

jklymakOct 4, 2018

Choose a reason for hiding this comment

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

Fixed

jklymak force-pushed thefix-correct-get-position-errors branch 2 times, most recently froma518ddf tod4c7069Compare

October 4, 2018 14:58

jklymak added2 commits

October 4, 2018 08:14

FIX: errors in get_position changes

c272624

TST: test that positioning works correctly

78db75f

TST: add image grid test

jklymak force-pushed thefix-correct-get-position-errors branch fromd4c7069 to78db75fCompare

October 4, 2018 15:15

anntzer reviewed

Oct 10, 2018

View reviewed changes

lib/mpl_toolkits/axes_grid1/parasite_axes.py

		@@ -327,9 +327,10 @@ def _remove_method(h):
		return ax2

		def get_tightbbox(self, renderer, call_axes_locator=True):
		bbs = [ax.get_tightbbox(renderer, call_axes_locator)
		bbs = [ax.get_tightbbox(renderer, call_axes_locator=call_axes_locator)

Copy link

Contributor

anntzerOct 10, 2018

Choose a reason for hiding this comment

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

I don't see how this change is needed? (doesn't hurt, but still?)

jklymak mentioned this pull request

Oct 12, 2018

ImageGrid in 3.0#12505

Closed

tacaswell merged commitff53308 intomatplotlib:master

Oct 12, 2018

Copy link

lumberbot-appbot commentedOct 12, 2018

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v3.0.x$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 ff533081242308803ecdad75cd2539c999278e66

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #12363: FIX: errors in get_position changes'

Push to a named branch :

git push YOURFORK v3.0.x:auto-backport-of-pr-12363-on-v3.0.x

Create a PR against branch v3.0.x, I would have named this PR:

"Backport PR#12363 on branch v3.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free tosuggest an improvement.

lumberbot-appbot added the status: needs manual backport label

Oct 12, 2018

QuLogic removed the status: needs review label

Oct 12, 2018

jklymak pushed a commit to jklymak/matplotlib that referenced this pull request

Oct 12, 2018

Merge pull requestmatplotlib#12363from jklymak/fix-correct-get-posi…

547c5ab

…tion-errorsFIX: errors in get_position changes

QuLogic added a commit that referenced this pull request

Oct 13, 2018

Merge pull request#12512from jklymak/auto-backport-of-pr-12363-on-v…

b8995e0

…3.0.xBackport PR#12363 on branch v3.0.x

QuLogic removed the status: needs manual backport label

Oct 13, 2018

jklymak deleted the fix-correct-get-position-errors branch

October 22, 2018 17:41

jklymak added the topic: geometry managerLayoutEngine, Constrained layout, Tight layout label

Oct 24, 2018

This was referencedOct 24, 2018

transiently linked interactivity of unshared pair of axes generated with make_axes_locatable#12613

Closed

make_axes_locatable : Colorbar in the middle instead of bottom while saving a pdf, png.#12624

Closed

FIX: allow non bbox_extra_artists calls#12635

Merged

Labels

Release critical

For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

topic: geometry manager

LayoutEngine, Constrained layout, Tight layout

7 participants

Movatterモバイル変換

Uh oh!

FIX: errors in get_position changes#12363

FIX: errors in get_position changes#12363

Uh oh!

Conversation

jklymak commentedOct 1, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

ImportanceOfBeingErnest commentedOct 1, 2018

Uh oh!

jklymak commentedOct 2, 2018

Uh oh!

timhoffm commentedOct 3, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

jklymak commentedOct 3, 2018

Uh oh!

timhoffm commentedOct 3, 2018

Uh oh!

jklymak commentedOct 3, 2018

Uh oh!

WeatherGod left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lumberbot-appbot commentedOct 12, 2018

Uh oh!

Uh oh!

jklymak commentedOct 1, 2018•
edited
Loading

timhoffm commentedOct 3, 2018•
edited
Loading