Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH add an inset_axes to the axes class #11026

Merged
merged 1 commit into from Jul 20, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Apr 11, 2018

PR Summary

and we are back to inset_axes....

@efiring suggested just going back to inset_axes. Since that was what we started with, I didn't object. The original argument for the longer name was if the API was to include a loc='SW' API, instead of the bounds API I've used here. Thats still possible by type-checking the bounds argument.

EDIT:

Name bikeshedding:

Proposal after comments below:

Axes.inset_axes_from_bounds
Axes.indicate_inset_bounds
Axes.indicate_inset_zoom

rect was too ambiguous, lbwh too obscure, bounds is already used for bboxes (as is extents for lbrt)...

Comments on names still welcome...

Old PR descr

Adds ax.inset_axes_from_rect. Pretty straight forward, but note that the transData one moves under zoom and pan.

Note that I chose not to go with the bbox_to_anchor, and loc approach. I understand that stuff, but it seems needlessly complicated. Maybe as a refinement.

import matplotlib.pyplot as plt

fig, ax = plt.subplots()
ax.plot(range(10))
axin1 = ax.inset_axes_from_rect([0.8, 0.1, 0.15, 0.15])
axin2 = ax.inset_axes_from_rect([5, 7, 2.3, 2.3], transform=ax.transData)
plt.show()

figure_1

Getting closer: This is a duplication of https://matplotlib.org/gallery/axes_grid1/inset_locator_demo2.html#sphx-glr-gallery-axes-grid1-inset-locator-demo2-py

import matplotlib.pyplot as plt

import numpy as np

def get_demo_image():
    from matplotlib.cbook import get_sample_data
    import numpy as np
    f = get_sample_data("axes_grid/bivariate_normal.npy", asfileobj=False)
    z = np.load(f)
    # z is a numpy array of 15x15
    return z, (-3, 4, -4, 3)

fig, ax = plt.subplots(figsize=[5, 4], constrained_layout=True)
Z, extent = get_demo_image()
Z2 = np.zeros([150, 150], dtype="d")
ny, nx = Z.shape
Z2[30:30 + ny, 30:30 + nx] = Z

# extent = [-3, 4, -4, 3]
ax.imshow(Z2, extent=extent, interpolation="nearest",
          origin="lower")

axins = ax.inset_axes_from_rect([0.5, 0.5, 0.47, 0.47])
axins.imshow(Z2, extent=extent, interpolation="nearest",
          origin="lower")
# sub region of the original image
x1, x2, y1, y2 = -1.5, -0.9, -2.5, -1.9
axins.set_xlim(x1, x2)
axins.set_ylim(y1, y2)
axins.set_xticklabels('')
axins.set_yticklabels('')

ax.zoom_inset_indicator(axins)
fig.canvas.draw()
plt.show()

figure_1

EDIT: methods renamed inset_axes_rect to allow other APIs in the future.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@jklymak jklymak added this to the v3.0 milestone Apr 11, 2018
@jklymak jklymak force-pushed the enh-axes-add-axes branch 2 times, most recently from ea75792 to 0d3425a Compare April 16, 2018 21:16
@ImportanceOfBeingErnest
Copy link
Member

In how far is it bearable or confusing for users to have

  • an axes_grid1.inset_locator.inset_axes which creates a axes_grid1.parasite_axes.AxesHostAxes
  • an axes.Axes.inset_axes which creates a .axes.Axes ?

(I could imagine the problem arising because people are used to have the same function at different places, e.g. plt.plot vs. axes.Axes.plot)

@jklymak
Copy link
Member Author

jklymak commented Apr 16, 2018

mplot3d call to plot objects return a different object than core plot. I don't personally think it should be a problem...

@jklymak jklymak changed the title WIP/ENH add an inset_axes to the axes class ENH add an inset_axes to the axes class Apr 17, 2018
@jklymak
Copy link
Member Author

jklymak commented Apr 19, 2018

I am done w/ this for now, and it can be reviewed if anyone has any suggestions. (note looks like a spare debug change got into the PR, but I'll remove after/during review).

@@ -23,7 +23,7 @@
x = r * np.sin(theta)
y = r * np.cos(theta)

ax.plot(x, y, z, label='parametric curve')
l = ax.plot(x, y, z, label='parametric curve')
Copy link
Contributor

Choose a reason for hiding this comment

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

remove change

"""

def __init__(self, rect, trans, parent):
self._rect = rect
Copy link
Contributor

Choose a reason for hiding this comment

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

just create the transformedbbox here?


def __call__(self, ax, renderer):
rect = self._rect
bbox = mtransforms.Bbox.from_bounds(rect[0], rect[1],
Copy link
Contributor

Choose a reason for hiding this comment

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

would typically write from_bounds(*rect), not that it really matters

# This puts the rectangle into figure-relative coordinates.
bb = _inset_locator(rect, transform, self)(None, None)
# if not set, set to the same zorder as legend
zorder = kwargs.pop('zorder', 5)
Copy link
Contributor

@anntzer anntzer Apr 19, 2018

Choose a reason for hiding this comment

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

kwargs.setdefault("zorder", Legend.zorder)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I don't think I want it tied to the legend value. And legend's value is hard coded i.e. not an rcParam....


zorder : number
Defaults to 5 (same as `.Axes.legend`). Adjust higher or lower
to change whether it is above or below data plotted on the axes.
Copy link
Contributor

Choose a reason for hiding this comment

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

"... on the parent axes" (or however you call it)

coordinates.

facecolor : Matplotlib color
Facecolor of the rectangle (default 'none')
Copy link
Contributor

Choose a reason for hiding this comment

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

full stops at end of lines (same below)

is '0.5'

alpha : number
Transparency of the rectangle and connector lines. Default 0.5
Copy link
Contributor

Choose a reason for hiding this comment

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

default is

-------

rectangle_patch, connector_lines :
`.Patches.Rectagle`, (four-tupple `.Patches.ConnectionPatch`) one
Copy link
Contributor

Choose a reason for hiding this comment

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

Rectangle tuple

# decide which two of the lines to keep visible....
pos = inset_ax.get_position()
bboxins = pos.transformed(self.figure.transFigure)
rectbbox = mtransforms.Bbox.from_bounds(rect[0], rect[1],
Copy link
Contributor

Choose a reason for hiding this comment

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

from_bounds(*rect)

-------

rectangle_patch, connector_lines :
`.Patches.Rectagle`, (four-tupple `.Patches.ConnectionPatch`) one
Copy link
Contributor

Choose a reason for hiding this comment

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

rectangle, tuple

@@ -1048,6 +1048,7 @@ def cla(self):
self.tables = []
self.artists = []
self.images = []
self.child_axes = []
Copy link
Contributor

Choose a reason for hiding this comment

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

stash them into self.artists? I'd rather work towards unifying all these containers rather than adding more of them...

Copy link
Member Author

Choose a reason for hiding this comment

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

Seec omment below for why not implimented....

@ImportanceOfBeingErnest
Copy link
Member

What I definitely like better about the axes_grid1.inset_locator.inset_axes is that you get some automatic padding, similar to a legend.
Here, any padding is either in percent of the parent axes width/height (i.e. the position would shift if the axes size changes) or you need to create an offset transform from scratch to supply to the transform argument.
So the equivalent of

from mpl_toolkits.axes_grid1.inset_locator import inset_axes

axins = inset_axes(ax, width="40%", height="30%")

would be

from matplotlib.transforms import ScaledTranslation

offset = ScaledTranslation(-5/72., -5/72., fig.dpi_scale_trans)
trans = ax.transAxes + offset
ax.inset_axes([.6,.7,.4,.3], transform=trans)

which is a bit cumbersome to write for such standard case.

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

If that's a desirable feature (not certain yet, but possibly), certainly it should be pulled to core so that legend, text annotations and whatnot can also benefit from it?

@ImportanceOfBeingErnest
Copy link
Member

@anntzer I think in its current form axes_grid1.inset_locator.inset_axes should not simply be pulled over, because it has some design flaws which become apparent once you leave the standard use case.
You may read #11060 and links therein to see what is confusing about it (and also review if you like ;)).

So for the core matplotlib, one should probably spend some more thoughts about how to best design such function to be powerful, but not as confusing.

@jklymak
Copy link
Member Author

jklymak commented Apr 19, 2018

@anntzer and @ImportanceOfBeingErnest, thanks for the feedback. I think the logic for padded boxes is indeed in .offsets.AnchoredOffsetBox.

.offsets.AnchoredOffsetBox works from the idea that you want to put the box somewhere semi-automatically inside an axes (or other bbox). In that case a padding makes some sense. But I find the whole API hard to deal with if I just want to put a box (or axes) somewhere deterministic on the figure or parent_axes. So here, I was trying to be more deterministic with a more streamlined API.

What might make sense is to add a "pad" parameter in appropriate units (probably points, versus pixels), that would offset the new inset axes from the rectangle provided. Then if you want upper right you do:

w = 0.2, h=0.2
axins = ax.inset_axes([1-w, 1-h, w, h], pad=5)

I don't think thats hard to do, though I'll have to check how its done to be robust under figure size changes. Could have an hpad and wpad if desired. I'd tend to be against different pads left and right, up and down...

Not sure how all that would work with set aspect ratios for the child axes.

Would something like that satisfy the need?

Conversely, I'm not 100% against just moving the inset_axes into core. But I really don't like the API, so felt this would be a decent time to change it. I appreciate some power users have figured out how to make it work, and recent attempts to improve the documentation have been heroic, but...

EDIT: cross-post!

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

To be clear, I think this should go in without padding support, because it's basically using the same positioning syntax as text, which is a good thing. Padding support should get its own discussion (and I'm not even certain it's something worth the complexity).

@jklymak
Copy link
Member Author

jklymak commented Apr 19, 2018

I guess personally I don't have a problem with padding just being a fraction of the axes size (i.e. I can hard code it into rect), but I can see why it is nice to have in physical units for consistency between different sized-axes. But I'm more than happy to leave that for a future PR, either as a new method or by enhancing the API of these methods. I don't think there is anything architecturally that needs to change to make that work.

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Apr 19, 2018

I'm thinking more in terms of ease of use. One can always argue that if people want to have full control they can reach their goal via the axes_grid1 inset or custom transforms etc.
So I'm comparing to legend here, because that is similar in the sense that you have a box with some extra information in. You may call legend() without any argument and get a nice legend. This has some padding to the outside and if you don't like the position you'd add loc=4 at most. Apart it's still deterministic and fully customizable, using all the remaining arguments.

In that sense, something like

def inset_axes(loc="upper right", pad=0.5, width=0.4, height=0.3, box=None, transform=None):

which can be called just as ax.inset_axes() could be more desireable.

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

I think "loc=number" is a terrible API. At least that should be using strings (which are also allowed with legend, but I would want new APIs to only use strings).

@jklymak
Copy link
Member Author

jklymak commented Jun 20, 2018

ping @efiring and @timhoffm if the new name suits. We could discuss on Monday's dev call, except I'll be unavailable. Happy to chat it out on gitter as well...

@jklymak
Copy link
Member Author

jklymak commented Jul 4, 2018

Rebased and squashed. Be nice to get this in relatively soon because then I can move onto axes for secondary scales...

@jklymak jklymak mentioned this pull request Jul 6, 2018
5 tasks
@jklymak jklymak closed this Jul 8, 2018
@jklymak jklymak reopened this Jul 8, 2018
@jklymak
Copy link
Member Author

jklymak commented Jul 8, 2018

recycled for CI

@jklymak
Copy link
Member Author

jklymak commented Jul 10, 2018

rebased

@jklymak
Copy link
Member Author

jklymak commented Jul 18, 2018

This just needs one more review to go in as a pretty useful 3.0 feature. OTOH, understand if it needs to get pushed 6 months...

_parent = parent

def inset_locator(ax, renderer):
bbox = _rect
Copy link
Member

Choose a reason for hiding this comment

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

Is this line actually doing anything other than renaming a variable? I think it is superfluous.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I would like to see this go in ASAP, preferably with one of the variations on naming that was discussed: drop the "from...", so it is "inset_axes()" and "indicate_inset()", and use "bounds" consistently for the corresponding variable name in private as well as public functions.

@jklymak jklymak changed the title ENH add an inset_axes_from_bounds to the axes class ENH add an inset_axes to the axes class Jul 20, 2018
@jklymak jklymak force-pushed the enh-axes-add-axes branch 2 times, most recently from 82b88b7 to 35c29c2 Compare July 20, 2018 04:44
@jklymak
Copy link
Member Author

jklymak commented Jul 20, 2018

Rebased and renamed to plain old inset_axes per @efiring. @timhoffm that may invalidate your approval? I'll leave it to the maintainers whether this has enough approval...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Development

Successfully merging this pull request may close these issues.

None yet

6 participants