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

tight_bbox made assumptions about the display-units without checking the figure #1138

Closed
wants to merge 1 commit into from

Conversation

pwuertz
Copy link
Contributor

@pwuertz pwuertz commented Aug 22, 2012

This PR tries to fix #1135. Tight_bbox tries to determine the bounding box of figure contents and saves a new figure that is shrunk to the new size. When saving a PDF using the PGF backend (which internally uses display-units) the scaling is wrong.

For rescaling the figure, adjust_bbox_pdf does some strange unit conversions I do not understand while adjust_bbox_png just uses the scaling matrix from the figure. Using fig.dpi_scale_trans works just fine for the PGF, PDF, SVG and EPS backend, so I removed the adjust_bbox_pdf function altogether.

There are no tests for bbox_inches and I don't really understand what bbox_inches is doing, so this needs some review.

@leejjoon
Copy link
Contributor

I do not remember the details why I had two separate functions, but obviously there was cases where "adjust_bbox_png" does not work for pdf, ps and svg backend. Unfortunately, I cannot remember which were those cases. I thought rasterization were involved, but my simple test seem to suggest that "adjust_bbox_png" also work with rasterization.

However, as a original author of the "tight_bbox" option, I am a little hesitant in accepting this PR as there may still exist some cases where "adjust_bbox_png" does not work as expected.

I believe another solution for this is to set the figure dpi to 72 before saving as pgf. This is what pdf backend does and I think this was main reason why we had "adjust_bbox_png" as a separate function.
So, for consistency, I think we should change the dpi to 72 before saving as pgf (or at least for saving pdf w/ pgf backend). And I think this will solve the tight_bbox issue on pgf backend as a side-effect.

So, my proposal is,

for 1.2, we set the figure dpi to 72 before saving as pgf

and after 1.2 is out,

we get rid of "adjust_bbox_pdf" in the dev branch and see if everything works.

@pwuertz
Copy link
Contributor Author

pwuertz commented Aug 27, 2012

But isn't the dpi an essential setting the user might want to control? Savefig.dpi affects the resolution of images embedded in the figures, why should a backend override this?

@leejjoon
Copy link
Contributor

In the default pdf backend, the original Figure.dpi value is saved as image_dpi before overriding the Figure.dpi, and the renderer uses image_dpi for the resolution of images (ps backend behaves similarly). I am not in a position to tell why this is implemented this way.

Still, it could be a bug of tight_bbox-related routines. And looking at the code, "adjust_bbox_pdf" does not make much sense to me and I do not remember why this is made so unfortunately. This could have been my oversight, or this could have something to do with rasterizaition as it (temporarily) changes the figure dpi.

What I am suggesting is that we try to fix this after 1.2 is released.

@pwuertz
Copy link
Contributor Author

pwuertz commented Aug 28, 2012

All right, I'm closing this PR in favor of a temporary workaround for PGF:
#1155

Is it possible that the tight_bbox functionality pre-dates the tight_layout function? They are very similar, but implemented in completely different ways. For example, the sole purpose of the first fake saving process in bbox_inches=='tight' is to get hands on a renderer instance. Tight_layout does this by just simply calling get_renderer from the backend. Both implementations now try to determine the sizes of all figure elements for the given canvas. After that, tight_bbox creates smaller figure and tight_layout makes the plot fill the figsize. This is essentially the functionality a single unified layout manager should take care of.

@pwuertz pwuertz closed this Aug 28, 2012
@leejjoon
Copy link
Contributor

Yes, the tight_bbox functionality pre-dates the tight_layout function. While they are very similar, but they are implemented (slightly) differently as they expect different side-effect. bbox_inches="tight" does not change the location of artists in the figure, but tight_layout does. tight_layout is intended for interactive use, thus it assume that figure is already drawn. This cannot be assumed for bbox_inches="tight". Of course, it will be good to have a layout manager for matplotlib!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems with bbox_inches='tight'
2 participants