8000 Give `AnnotationBbox` an opinion about its extent by ImportanceOfBeingErnest · Pull Request #13457 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Give AnnotationBbox an opinion about its extent #13457

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 27, 2020
Merged

Give AnnotationBbox an opinion about its extent #13457

merged 1 commit into from
May 27, 2020

Conversation

ImportanceOfBeingErnest
Copy link
Member
@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Feb 18, 2019 •
edited
Loading

PR Summary

Fixes #12699.

Give AnnotationBbox an opinion about its extent, i.e. add a get_window_extent and get_tightbbox.
This allows e.g. to savefig with the bbox_inches='tight' option and not have the annotations cropped.

Before

annbbox_3 0 2

With this PR

annbbox_3 0 0 post1019 g3cc1af35a

Code 
import matplotlib
v = matplotlib.__version__
import matplotlib.pyplot as plt

import numpy as np

from matplotlib.patches import Circle
from matplotlib.offsetbox import (TextArea, DrawingArea, OffsetImage,
                                  AnnotationBbox, AnchoredOffsetbox)

fig, ax = plt.subplots(figsize=(4,3), constrained_layout=False)

xy = [.5, .5]
ax.plot(xy[0], xy[1], ".r")

for x0 in [-0.2, 1.1]:
    da = DrawingArea(20, 20, 0, 0, clip=x0<0)
    p = Circle((np.sign(x0)*20+10, 30), 32, fill=False, linewidth=3, edgecolor="crimson")
    da.add_artist(p)
    
    ab3 = AnnotationBbox(da, xy, xybox=(x0, 0.5), xycoords='data',
                        boxcoords="axes fraction", box_alignment=(0., .5),
                        arrowprops=dict(arrowstyle="->"))
    ax.add_artist(ab3)

im = OffsetImage(np.random.rand(30,30), zoom=1)
im.image.axes = ax
ab6 = AnnotationBbox(im, (0.5, -.3), xybox=(0.5, .2), xycoords='axes fraction',
                     boxcoords="axes fra
8000
ction", pad=0.3,
                     arrowprops=dict(arrowstyle="->"))
ax.add_artist(ab6)

#plt.tight_layout()
plt.savefig(f'annbbox_{v}.png', bbox_inches='tight', facecolor="#e1f3ec")    
plt.show()

It seems for static figures neither tight_layout, nor constrained_layout are affected by this. But using

constrained_layout=True and resizing the figure on screen shows some really weird behaviour.

bboxannot

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

@ImportanceOfBeingErnest ImportanceOfBeingErnest changed the title windowextent-for-annotationbbox Give AnnotationBbox an opinion about its extent Feb 18, 2019
@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the windowextent-for-annotationbbox2 branch from 575a855 to 3993fc5 Compare February 18, 2019 01:02
@jklymak
Copy link
Member
jklymak commented Feb 18, 2019

You can certainly imagine how 'axes fraction' and constrained layout could get into a bad interaction on dynamic resizing.

@ImportanceOfBeingErnest
Copy link
Member Author

I think any kind of messed up figure would be fine, I was more worried about the flickering.

@jklymak
Copy link
Member
jklymak commented Feb 18, 2019

I haven’t tested 8000 but I think it’s just jumping between two states

@jklymak jklymak self-assigned this Feb 18, 2019
@jklymak
Copy link
Member
jklymak commented Feb 18, 2019

Well, at least CL gives a warning: UserWarning: constrained_layout not applied. At least one axes collapsed to zero width or height. I'll have to look at why it doesn't like this. Looks hard, however. I don't think CL will easily work with offsetbox.

@jklymak
Copy link
Member
jklymak commented Feb 19, 2019 •
edited
Loading

OK, so 'axes fraction' is impossible unless the offset box is part of the constraint solver from the beginning. Note other co-ordinate systems are fine, but you can't adjust the size of the axes based on elements whose positions are not known until you adjust the size of the axes. If you used the constraint solver inside the offset boxes to make that constraint that could work, but otherwise this won't be compatible with constrained layout.

@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the windowextent-for-annotationbbox2 branch from 3993fc5 to e14c083 Compare February 21, 2019 02:02
@ImportanceOfBeingErnest
Copy link
Member Author

Ok, let's ignore constrained_layout.

I added a test. The test fails with tight_layout though. The reason is unclear to me. The very same function when run through python succeeds, but when run through pytest fails. The failure is significant; it seems that tight_layout isn't really applied, leading to a difference in positions of > 40 pixels, so it's not just some difference in styles.

@jklymak
Copy link
Member
jklymak commented Feb 22, 2019

Are you sure the renderer is still the correct one after the figsave?

@ImportanceOfBeingErnest
Copy link
Member Author
ImportanceOfBeingErnest commented Feb 22, 2019 •
edited
Loading

Not sure what would render a renderer incorrect?

So here is the problem statement: The following code when run inside this branch is working fine.

import io
import matplotlib
print(matplotlib.__version__)
matplotlib.use("Agg")
import matplotlib.pyplot as plt
import matplotlib.patches as mpatches
from matplotlib.offsetbox import (DrawingArea,AnnotationBbox)

def test_annotationbbox_extents_bug():
    
    plt.rcParams.update(plt.rcParamsDefault)
    fig, ax = plt.subplots(figsize=(4, 3), dpi=100)
    ax.axis([0, 1, 0, 1])

    da = DrawingArea(20, 20, 0, 0, clip=True)
    p = mpatches.Circle((10, 10), 10)
    da.add_artist(p)
    annot = AnnotationBbox(da, (0.5, -0.5), xybox=(0, 100),
                           xycoords='data',
                           boxcoords="offset points", pad=0.3,
                           arrowprops=dict(arrowstyle="->"),
                           annotation_clip=False)
    ax.add_artist(annot)

    # This line is critical:
    fig.savefig(io.BytesIO())                                          ### <------  critical 

    fig.tight_layout()
    fig.canvas.draw()
    renderer = fig.canvas.get_renderer()

    fig.savefig("annbboxbug.png", facecolor="papayawhip")
    assert annot.get_window_extent(renderer).y0 > 0
    
test_annotationbbox_extents_bug()

It gives this output

image

There is one critical line, which is the saving of the figure to a buffer before calling tight_layout.
If I remove this savefig line, the assertion fails. The output in this case is

image

I can replace savefig by fig.canvas.draw() to make it work or not work as well.
I suppose something in this savefig or draw is not happening when running through pytest such that this fails on the tests as well.
Any ideas?

In any case, additing an additional fig.canvas.draw() in the tests make it work.

@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the windowextent-for-annotationbbox2 branch from e14c083 to 6773da4 Compare February 22, 2019 23:36
@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the windowextent-for-annotationbbox2 branch from 6773da4 to 1c17991 Compare August 7, 2019 11:54
@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.2.0 milestone Aug 7, 2019
@ImportanceOfBeingErnest
Copy link
Member Author

After rebasing the test fails. Something must have changed in the way tight_layout works between February and now. Any ideas? (I don't think it's possible to bisect with a commit on top, or is it?)

@jklymak
Copy link
Member
jklymak commented Aug 7, 2019

I think bisect works fine no matter where the commits come from. Not sure what changed.

@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the windowextent-for-annotationbbox2 branch 2 times, most recently from 80bc6f1 to 74b1c51 Compare August 7, 2019 18:30
@ImportanceOfBeingErnest
Copy link
Member Author

Since tight_layout might be unreliable for artists positionned in axes coordinates anyways, I just put in a smoke test to make sure it doesn't fail.

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Sep 5, 2019
timhoffm
timhoffm approved these changes Mar 21, 2020
View reviewed changes
Copy link
Member
@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Should this get a what's new/change note? I'm undecided.

@QuLogic
Copy link
Member
QuLogic commented May 4, 2020

This needs a rebase if you want it in for 3.3.

@jklymak
Copy link
8000 Member
jklymak commented May 26, 2020

@ImportanceOfBeingErnest any interest in doing the rebase? This can be merged once thats done (modulo a what's new entry?)

@QuLogic QuLogic force-pushed the windowextent-for-annotationbbox2 branch from 74b1c51 to 01cb983 Compare May 27, 2020 04:17
@QuLogic
Copy link
Member
QuLogic commented May 27, 2020

This was a simple conflict of two tests being added at the end of a file, so I went ahead and rebased it. If you're alright with that, please can one of you merge it.

@jklymak jklymak merged commit 2f63dc4 into matplotlib:master May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees

@jklymak jklymak

Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Annotations get cropped out of figures saved with bbox_inches='tight'
5 participants
@ImportanceOfBeingErnest @jklymak @QuLogic @timhoffm @tacaswell
0