Join bbox_extra_artists and bbox_inches

[jenshnielsen]

Don't ignore the initial bbox when using bbox_extra_artist. Rather join the two bboxes. This should fix #1419

[dmcdougall]

With #1418, this works for tables (the original example in #1419).

[dmcdougall]

Great job, @jenshnielsen.

[pelson]

This looks fine to merge IMHO. Is there an easy test we could put in to catch this for the future?

[jenshnielsen]

I will look into making a test for the bbox stuff later today. I don't think an image comparison test makes much sense but we should be able to make a test of the size of the bbox_union of something like that.

[jenshnielsen]

This is a bug introduced in 2f11dee and only present on master. I'm not sure
about how to best revert it.

[jenshnielsen]

I have added a test of bbox_inches='tight'. To do this I adapted the image comparison decorator to take a dict that is then passed to the savefig method, this should also make it possible to pass other arguments to the savefig function i.e. dpi or transparent. I'm not sure this is the best solution so if anyone have any better idea I will be happy to adopt it.

The test make a figure with a table and and external legend and should verify that the clipping is right and prevent both the issue in #1420 and #1425 from popping up again. In addition I have merged #1421 into this one to clip the table correctly. Furthermore I cherry-picked #1425 into this one. That should be merged from 1.2.x rather than this one.

[jenshnielsen]

There is something wrong with the tests right now. When running all tests these bbox tests are intermixed with the
axes tests. I will investigate this and fix it tomorrow.

[minrk]

For public record, this also fixes another symptom I recently started seeing, where the bbox is totally wrong on any figure with a legend:

plt.plot([1,3,2])
plt.legend(["line"])
plt.gcf().savefig('fig.png', bbox_inches='tight')

gives this with master (4e6b2c0):

and behaves exactly as expected after applying this patch.

[jenshnielsen]

It should be working now. A stupid indention bug in the test caused the problem with the axes test.

[jenshnielsen]

Is there a way to see more verbose output from travis? It is difficult to see if new tests are actually being run by travis
or fail as a knowfail.

[pelson]

Is it critical that you have this version of freetype? What happens if you don't specify this?

[jenshnielsen]

Lets try to remove it. It the test fails for any one we can add it back in or remove some of the text.

[pelson]

Is there a way to see more verbose output from travis?

The only approach I know of would be to change the tests.py script to make it more verbose throughout. That might not be a bad idea in general...

[jenshnielsen]

Another user reported this on the IPython mailing list http://mail.scipy.org/pipermail/ipython-dev/2012-October/010519.html It would be good if we can get the fix in soon. If needed the test can wait. Is there anything else that needs to be done?

[jenshnielsen]

Can we please soon merge the fix for this issue. In total we now have 2 issue reports from IPython, 1 from matplotlib and a question on the IPython mailing list all about this one simple issue.

[piti118]

I can confirm that it works as advertised. Thanks a bunch @jenshnielsen.

[pelson]

👍 I'd be happy to merge this onto master. I will leave it a couple of days for others to comment, nut IMHO this is good to go.

Thanks @jenshnielsen

[jenshnielsen]

Would it be possible to merge 1.2.x onto master again soon so this one does not need to carry 77d965d
which is already on 1.2.x and in 1.2.rc3 in order not to cause any problems when merging.

[pelson]

Would it be possible to merge 1.2.x onto master again soon so this one does not need to carry 77d965d

Have done. I don't quite follow how it would cause any problems (if they are one and the same commit). As it stands, this PR would merge without a problem.

Cheers,

[jenshnielsen]

Thanks. God to know that it is not an issue. Guess I was just underestimating the capacity of git.

[jenshnielsen]

Just pushed a small update. We don't need the if statement if savefig_kwarg defaults to a empty dict instead of none.

[jenshnielsen]

I see your point. I think it's save but there is no reason to risk any issues so lets do it like this. It should be similar to the other default arguments.

[jenshnielsen]

@dmcdougall Yes that's what I did in 6d585f3 and what was done before 63733aa the only difference now compared to the original is that instead of using an if else statement to do calls with different arguments to savefig, savefig_kwarg is now set to an empty dict inside the function if it is None when called. This is similar to how extensions are handled and more readable imho.

[pelson]

Ok. #1443 suggest that this PR should not go in master, but instead should be included in v1.2.x (for a v1.2.0 release).

@mdboom : You should have visibility of this. Do you agree that this needs to be bumped up the priority list (and re-based against 1.2.x)?

[jenshnielsen]

I don't think that commit was merged into 1.2.x . That line is different in 1.2.x and the pull request #1345 only merged into master

[pelson]

I don't think that commit was merged into 1.2.x

Ah, your right. Its funny how you have to see the pull request to see which branch the commit was applied to. Thanks @jenshnielsen .

[pelson]

Good stuff @jenshnielsen : Thanks for your hard work and patience!

Cheers,