FIX: errors in get_position changes by jklymak · Pull Request #12363 · matplotlib/matplotlib · GitHub

PR Summary

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 is Flake 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

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

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

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

...rebased

flake8 says

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

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

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

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?

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

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))

Ummm, OK, but really 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

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

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.)

I think a list comprehension would be more readable anyway.

OK, list comprehension is bearable.

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())

Fine

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

Fixed

TST: add image grid test

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

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 to suggest an improvement.

…tion-errors FIX: errors in get_position changes

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

jklymak added status: work in progress and removed status: work in progress labels Oct 1, 2018

jklymak added this to the v3.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 the fix-correct-get-position-errors branch from 59a994b to a5e6667 Compare 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

timhoffm added the status: needs rebase label Oct 3, 2018

jklymak force-pushed the fix-correct-get-position-errors branch from a5e6667 to 0ce489c Compare October 3, 2018 15:26

jklymak removed the status: needs rebase label Oct 3, 2018

jklymak force-pushed the fix-correct-get-position-errors branch from 0ce489c to 1da1c35 Compare October 3, 2018 17:49

timhoffm approved these changes Oct 3, 2018

View reviewed changes

WeatherGod reviewed Oct 4, 2018

View reviewed changes

jklymak force-pushed the fix-correct-get-position-errors branch 2 times, most recently from a518ddf to d4c7069 Compare October 4, 2018 14:58

jklymak added 2 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 the fix-correct-get-position-errors branch from d4c7069 to 78db75f Compare October 4, 2018 15:15

anntzer reviewed Oct 10, 2018

View reviewed changes

jklymak mentioned this pull request Oct 12, 2018

ImageGrid in 3.0 #12505

Closed

tacaswell merged commit ff53308 into matplotlib:master Oct 12, 2018

lumberbot-app Bot added the Still 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 request matplotlib#12363 from jklymak/fix-correct-get-posi…

547c5ab

…tion-errors FIX: errors in get_position changes

QuLogic added a commit that referenced this pull request Oct 13, 2018

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

b8995e0

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

QuLogic removed the Still 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 manager LayoutEngine, Constrained layout, Tight layout label Oct 24, 2018

                           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))

                              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))

                   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)

FIX: errors in get_position changes by jklymak · Pull Request #12363 · matplotlib/matplotlib · GitHub

Repository navigation

FIX: errors in get_position changes - #12363

FIX: errors in get_position changes#12363tacaswell merged 2 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom jklymak:fix-correct-get-position-errorsjklymak/matplotlib:fix-correct-get-position-errorsCopy head branch name to clipboard

Conversation

jklymak commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

ImportanceOfBeingErnest commented Oct 1, 2018

Uh oh!

jklymak commented Oct 2, 2018

Uh oh!

timhoffm commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak commented Oct 3, 2018

Uh oh!

timhoffm commented Oct 3, 2018

Uh oh!

jklymak commented Oct 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-app Bot commented Oct 12, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

FIX: errors in get_position changes#12363
tacaswell merged 2 commits into
matplotlib:masterfrom
jklymak:fix-correct-get-position-errors

jklymak commented Oct 1, 2018 •
edited
Loading

timhoffm commented Oct 3, 2018 •
edited
Loading