Fix inset_axes + doc #11060

ImportanceOfBeingErnest · 2018-04-16T16:54:37Z

PR Summary

This PR fixes the mpl_toolkit's inset_axes, which got broken via #10756, and adds documentation as well as examples.

More in detail:
As seen from #8120 as well as #8952, inset_axes can be confusing. While #8952 is solved simply by knowing that in order to use relative units, a 4-tuple must be given, #8120 results from confusion about the transform being None and hence being the display units.
The specific case from #8120 has been "fixed" in #10756 by changing the default transform. Unfortunately noone noticed that this broke all the usual intended behaviour of inset_axes. Two tests which were introduced in #10756 did not actually assert those default cases.
An attempt by @jklymak in #10986 to fix the broken behaviour was too stingent and would have broken other previously working cases.

This PR:

Reconstitutes the behaviour from before the wrong "fix". inset_axes remains completely backward compatible.
Adds errors and warnings for the cases that would cause seemingly undesired behaviour, but result from a wrong usage of the function.
Adds a text which tests the position of the inset_axes itself for many of the possible use cases. This should ensure that this function does not get broken easily again. The correct raise of errors and warnings is equally tested.
Enhances the docs:
- The inset_axes docstring is significantly extended.
- Adds examples: There were previously two example pages about inset_locator. One showing a simple inset and a zoomed inset with a bar. The other showing a zoomed inset with an image. I moved the zoomed inset with bar to the latter example, making this the "zoomed inset example". I then extended the former example by some more use cases, making sure to cover a good cross section of how this function can be used.

The following table summarizes the possible use cases and shows in how far they are now documented and tested.

PR Checklist

Code is completely backward compatible
Has Pytest style unit tests
Code is PEP 8 compliant
Old features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant

jklymak · 2018-04-16T18:01:18Z

I support in general! Sorry if my attempts to fix things made them worse. Thanks for taking a close and careful look

jklymak

This looks great, but some minor wording suggestions and clarifications. Thanks!

jklymak · 2018-04-19T17:18:14Z

lib/mpl_toolkits/axes_grid1/inset_locator.py

+        Bbox that the inset axes will be anchored to. If None,
+        *parent_axes.bbox* is used. If a tuple, can be either
+        [left, bottom, width, height], or [left, bottom].
+        If *width* and/or *height* are given in relative units, the 2-tuple


"If the kwargs width and height are specified ..." Otherwise not clear that you are not referring to the width and height just mentioned.

jklymak · 2018-04-19T17:19:33Z

lib/mpl_toolkits/axes_grid1/inset_locator.py


    bbox_transform : `matplotlib.transforms.Transform`, optional
-        Transformation for the bbox. if None, `parent_axes.transAxes` is used.
+        Transformation for the bbox. If None, a `.transforms.IdentityTransform`


...for the bbox that contains the inset axes.

"... is used (i.e. pixel co-ordinates). "

jklymak · 2018-04-19T17:21:33Z

lib/mpl_toolkits/axes_grid1/inset_locator.py

-        Transformation for the bbox. if None, `parent_axes.transAxes` is used.
+        Transformation for the bbox. If None, a `.transforms.IdentityTransform`
+        is used. This is useful when not providing any argument to
+        *bbox_to_anchor*. When using *bbox_to_anchor* it almost always makes


I don't understand whats useful about specifying "None", when not specifying a bbox_to_anchor? In that case surely the bbox_to_anchor is the parent_axes and the transform is transAxes?

That's the default case: inset_axes(parent_axes, width="40%", height="30%"). I suppose that is how people want to use this most often. In that case, bbox_to_achor is parent_axes.bbox and bbox_transform is None, which is the transforms.IdentityTransform(). This is not transAxes.

Width and height get turned into pixels?

Well, width and height are either in inches or relative to the bounding box (percentage). At the end of the day, everything is turned into pixels.

Concerning the bounding box, if bbox_transform is None, the IdentityTransform() is used. That means that the bbox_to_anchor is in pixels. This makes sense, because the standard parent_axes.bbox is e.g.

(x0=0.125, y0=0.11, x1=0.9, y1=0.88), transformed to (x0=0.0, y0=0.0, x1=6.4, y1=4.8), transformed by the dpi of 100, resulting in

(80.0, 52.8, 496.0, 369.6) which is in pixels.

I don't think anything should be thought of as pixels until draw time. Axes bboxes are in figure-relative units (as they should be), and not converted until draw time to pixels. Because at draw time, the dpi changes more often than not. See all the inconsistencies in the docs and code that have to do w/ dpi. I really don't think the bbox_transform should default to None here, but rather to the units consistent with the default for bbox_to_anchor which is figure-coordinates. But I appreciate this was the historical way it was done, so we are stuck with it...

Well, first of all Axes bboxes are TransformedBboxes. So they are somehow transient until draw time. But then they evaluate to pixels. This is not a problem at all. The problem only occurs when allowing the user to specify some other bounding box. Since the user usually won't create a TransformedBbox, but rather use a static tuple here, they would usually need to set a different transform as well. But we wouldn't want to include the complete logic of guessing what the user wants inside of this function, right? Like "if a bbox_to_anchor is specified and it is a tuple, chances are 98% that the user would have wanted to use the ax.transAxes, so let's change this silently." is not an option.

I would agree that if writing such function again, one may come up with a better way, possibly simply not allowing for width/height in inches, therefore being able to use a different default transform altogether etc. The aim of this PR is to restore the old behaviour. Anything else can go outside of axes_grid1 into whatever new fancy features to come.

jklymak · 2018-04-19T17:24:08Z

lib/mpl_toolkits/axes_grid1/inset_locator.py

+        sense to also specify a *bbox_transform*. This might often be the
+        axes transform *parent_axes.transAxes*. Inversely, when specifying
+        the axes- or figure-transform here, make sure to provide a bounding
+        box to *bbox_to_anchor* as well, else the result may be unexpected.


This is too vague. What bbox is used if the user doesn't specify bbox_to_anchor? Maybe:

"...figure-transform here, be aware that not specifying bbox_to_anchor will use parent_axes.bbox, the units of which are in figure.transFigure"

I'm now doing:

Inversely, when specifying the axes- or figure-transform here, be aware that not specifying
bbox_to_anchor will use parent_axes.bbox, the units of which are
in display (pixel) coordinates.

jklymak · 2018-04-19T17:24:38Z

lib/mpl_toolkits/axes_grid1/inset_locator.py

@@ -437,6 +465,8 @@ def inset_axes(parent_axes, width, height, loc=1,

    borderpad : float, optional
        Padding between inset axes and the bbox_to_anchor. Defaults to 0.5.
+        The units are font size, i.e. for a default font size of 10 points


are the axes fontsize

jklymak · 2018-04-19T17:25:30Z

lib/mpl_toolkits/axes_grid1/inset_locator.py

+    if bbox_transform in [parent_axes.transAxes,
+                          parent_axes.figure.transFigure]:
+        if bbox_to_anchor is None:
+            warnings.warn("Using the axes or figure transform requires to "


"requires a bounding..."

jklymak · 2018-04-19T17:25:57Z

lib/mpl_toolkits/axes_grid1/inset_locator.py


    bbox_transform : `matplotlib.transforms.Transform`, optional
-        Transformation for the bbox. if None, `parent_axes.transAxes` is used.
+        Transformation for the bbox. If None, a `.transforms.IdentityTransform`


jklymak

Thanks again!

ImportanceOfBeingErnest · 2018-05-03T17:29:32Z

@tacaswell This would be another example where it's not clear how to handle concerning backporting. This PR mostly enhances documentation and fixes a bug that has previously been introduced into master. In so far it is probably not worth backporting. On the other hand it fixes a critical sentence in the docs which was simply wrong and led to some confusion and even a wrong PR being merged to master based on this one sentence.
It seems that it could currently be backported without problem, if however six is removed, it would conflict. (Note that it would of course only make sense to backport, if the documentation revival of axes_grid1 is also backported, otherwise noone would benefit from that fixes sentence.)

jklymak · 2018-05-03T17:49:40Z

@ImportanceOfBeingErnest why would it conflict if six was removed and you backported? I don't see that your changes use six anywhere, but I didn't check carefully.

The confusing thing is that only the diff with the branch is ported so if your diff with master doesn't have anything that requires six, then your diff with 2.2.3 shouldn't either. Where we will run into difficulty is when you use a py3 feature that requires six to be able to work in py2.

ImportanceOfBeingErnest · 2018-05-03T18:11:50Z

Maybe I didn't understand it correctly. So what you are saying is that there will never be a diff between the file on master and the file on the other branch being calculated? Instead only the diff between my file and the one on master is directly transferred into the other branch?
If so the complete problem is of course much less severe.

ImportanceOfBeingErnest · 2018-05-16T23:29:35Z

Rebased.
This is neither urgent, nor critical, but it would be a pity if it was forgotten to be merged before the next release (thereby implicitely introducing the bug in current master).

ImportanceOfBeingErnest · 2018-07-01T13:53:56Z

Ping.
Just reminding that if this does not get merged before 3.0, a bug will be released.

phobson

This looks like a much needed improvement to me.

I'd like someone more familiar with axes_grid to mash the green button though.

lumberbot-app · 2018-07-02T16:30:47Z

There seem to be a conflict, please backport manually

jklymak · 2018-07-02T16:30:58Z

Happily mashed! (not that I'm claiming I'm more familiar than you - but "familiar enough"....

leejjoon · 2018-07-17T08:25:38Z

First of all, big thanks to @ImportanceOfBeingErnest and @jklymak (and others) for the effort.
Here are minor comments/idea to improve the docs (I used to be familiar with it as I wrote most of the relevant code, but maybe not anymore).

It may be better to explicitly say that the default value of bbox_transform is different from that of the legend.
could be too much detail, but may better explain what is going on: the value of bbox_to_anchor (or the return value of its get_points method) is transformed by the bbox_transform (Identity transform if None) and then interpreted as points in the display coordinate (which is dpi dependent).

ImportanceOfBeingErnest · 2018-07-17T10:50:19Z

@leejjoon It seems this makes sense. Would you have the bandwidth to do a PR on that? Or, if not, point to the places where you envision adding this?

leejjoon · 2018-07-18T05:48:49Z

Yes, I will work on the PR.

tacaswell · 2018-08-04T23:15:38Z

This does not backport cleanly and depends an other changes that are on master branch for the newtests to work.

ImportanceOfBeingErnest · 2018-08-05T00:15:15Z

The only thing that would strictly need to be backported here is the removal of the docstring saying

if None, parent_axes.transAxes is used.

because that sentence is blantly wrong. Everything else in this commit is a fix for an issue which was not present in 2.2, and a lot of enhancements for docstrings, tests and understandable errormessages.

Now because #11079 is equally not backported, the docstrings are not even shown in the 2.2 documentation builds.

tacaswell · 2018-08-05T00:38:45Z

ok, I'll make a more through attempt to backport both of them.

…inset_axes Fix inset_axes + doc Conflicts: examples/axes_grid1/inset_locator_demo.py - kept master branch version examples/axes_grid1/inset_locator_demo2.py - kept master branch version lib/mpl_toolkits/axes_grid1/inset_locator.py - conflicts around __future__ import

ImportanceOfBeingErnest force-pushed the fix-inset_axes branch 5 times, most recently from 3dd84b7 to a859dc0 Compare April 16, 2018 20:30

This was referenced Apr 16, 2018

axes_grid1 docs do not appear on mpl.org anymore. #9582

Closed

Inconsistent inset_axes position between show(), savefig(format='png') and savefig(format='pdf') #8120

Closed

jklymak reviewed Apr 19, 2018

View reviewed changes

jklymak added this to the v2.2.3 milestone Apr 19, 2018

jklymak added topic: mpl_toolkit Documentation labels Apr 19, 2018

ImportanceOfBeingErnest force-pushed the fix-inset_axes branch from a859dc0 to a10831c Compare April 19, 2018 18:04

jklymak approved these changes Apr 19, 2018

View reviewed changes

ImportanceOfBeingErnest force-pushed the fix-inset_axes branch from a10831c to fe2b396 Compare April 19, 2018 18:50

ImportanceOfBeingErnest mentioned this pull request Apr 19, 2018

ENH add an inset_axes to the axes class #11026

Merged

6 tasks

jklymak added the status: needs review label Apr 28, 2018

ImportanceOfBeingErnest force-pushed the fix-inset_axes branch 2 times, most recently from 9ba94a7 to 3225451 Compare April 30, 2018 20:38

ImportanceOfBeingErnest mentioned this pull request May 2, 2018

Unexpected behavior for Axes.quiver(transform=None) #11153

Closed

ImportanceOfBeingErnest force-pushed the fix-inset_axes branch from 3225451 to d5fe96f Compare May 16, 2018 22:23

Fix inset_axes + doc

60e9ec0

ImportanceOfBeingErnest force-pushed the fix-inset_axes branch from d5fe96f to 60e9ec0 Compare May 16, 2018 22:50

jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 1, 2018

phobson approved these changes Jul 2, 2018

View reviewed changes

jklymak merged commit 3696261 into matplotlib:master Jul 2, 2018

lumberbot-app bot added the status: needs manual backport label Jul 2, 2018

QuLogic removed the status: needs review label Jul 5, 2018

leejjoon mentioned this pull request Jul 23, 2018

minor doc update on axes_grid1's inset_axes #11744

Merged

6 tasks

tacaswell modified the milestones: v2.2.3, v3.0 Aug 4, 2018

tacaswell removed the status: needs manual backport label Aug 4, 2018

tacaswell modified the milestones: v3.0, v2.2.3 Aug 5, 2018

tacaswell added the status: needs manual backport label Aug 5, 2018

tacaswell mentioned this pull request Aug 5, 2018

Backport axes grid work #11813

Merged

tacaswell removed the status: needs manual backport label Aug 10, 2018

ImportanceOfBeingErnest mentioned this pull request Aug 11, 2018

inset_locator.inset_axes produces axes without extent and at incorrect position. #8952

Closed

ImportanceOfBeingErnest deleted the fix-inset_axes branch February 17, 2019 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inset_axes + doc #11060

Fix inset_axes + doc #11060

ImportanceOfBeingErnest commented Apr 16, 2018 •

edited

Loading

jklymak commented Apr 16, 2018

jklymak left a comment

jklymak Apr 19, 2018

jklymak Apr 19, 2018

jklymak Apr 19, 2018

ImportanceOfBeingErnest Apr 19, 2018

jklymak Apr 19, 2018

ImportanceOfBeingErnest Apr 19, 2018

jklymak Apr 19, 2018

ImportanceOfBeingErnest Apr 19, 2018

jklymak Apr 19, 2018

ImportanceOfBeingErnest Apr 19, 2018

jklymak Apr 19, 2018

jklymak Apr 19, 2018

jklymak Apr 19, 2018

jklymak left a comment

ImportanceOfBeingErnest commented May 3, 2018

jklymak commented May 3, 2018

ImportanceOfBeingErnest commented May 3, 2018

ImportanceOfBeingErnest commented May 16, 2018

ImportanceOfBeingErnest commented Jul 1, 2018

phobson left a comment

lumberbot-app bot commented Jul 2, 2018

jklymak commented Jul 2, 2018

leejjoon commented Jul 17, 2018

ImportanceOfBeingErnest commented Jul 17, 2018

leejjoon commented Jul 18, 2018

tacaswell commented Aug 4, 2018

ImportanceOfBeingErnest commented Aug 5, 2018

tacaswell commented Aug 5, 2018

Fix inset_axes + doc #11060

Fix inset_axes + doc #11060

Conversation

ImportanceOfBeingErnest commented Apr 16, 2018 • edited Loading

PR Summary

PR Checklist

jklymak commented Apr 16, 2018

jklymak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

ImportanceOfBeingErnest commented May 3, 2018

jklymak commented May 3, 2018

ImportanceOfBeingErnest commented May 3, 2018

ImportanceOfBeingErnest commented May 16, 2018

ImportanceOfBeingErnest commented Jul 1, 2018

phobson left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Jul 2, 2018

jklymak commented Jul 2, 2018

leejjoon commented Jul 17, 2018

ImportanceOfBeingErnest commented Jul 17, 2018

leejjoon commented Jul 18, 2018

tacaswell commented Aug 4, 2018

ImportanceOfBeingErnest commented Aug 5, 2018

tacaswell commented Aug 5, 2018

ImportanceOfBeingErnest commented Apr 16, 2018 •

edited

Loading