Join bbox_extra_artists and bbox_inches #1420

Merged
merged 10 commits into from Nov 2, 2012

Conversation

Projects
None yet
6 participants
@jenshnielsen
Member

jenshnielsen commented Oct 21, 2012

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

@dmcdougall

This comment has been minimized.

Show comment
Hide comment
@dmcdougall

dmcdougall Oct 21, 2012

Member

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

Member

dmcdougall commented Oct 21, 2012

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

@dmcdougall

This comment has been minimized.

Show comment
Hide comment
@dmcdougall

dmcdougall Oct 21, 2012

Member

Great job, @jenshnielsen.

Member

dmcdougall commented Oct 21, 2012

Great job, @jenshnielsen.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Oct 22, 2012

Member

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

Member

pelson commented Oct 22, 2012

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

@jenshnielsen

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 22, 2012

Member

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.

Member

jenshnielsen commented Oct 22, 2012

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 comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 22, 2012

Member

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

Member

jenshnielsen commented Oct 22, 2012

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

jenshnielsen added some commits Oct 20, 2012

Tables: Fix get_window_extent for table to allow table to be added to…
… bbox_extra_artists.

The get_window_extent is a method on the cells not the key of the cells.
If a table exists add it to default_bbox_extra_artists. This avoids c…
…lipping the

table if bbox_inches='tight' is used.
@jenshnielsen

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 24, 2012

Member

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.

Member

jenshnielsen commented Oct 24, 2012

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

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 24, 2012

Member

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.

Member

jenshnielsen commented Oct 24, 2012

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

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 24, 2012

Contributor

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):

legend

and behaves exactly as expected after applying this patch.

Contributor

minrk commented Oct 24, 2012

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):

legend

and behaves exactly as expected after applying this patch.

@jenshnielsen

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 25, 2012

Member

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

Member

jenshnielsen commented Oct 25, 2012

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

@jenshnielsen

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 25, 2012

Member

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.

Member

jenshnielsen commented Oct 25, 2012

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.

lib/matplotlib/tests/test_bbox_tight.py
+import numpy as np
+
+@image_comparison(baseline_images = ['bbox_inches_tight'], remove_text=True,
+ freetype_version = '2.4.10',

This comment has been minimized.

@pelson

pelson Oct 25, 2012

Member

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

@pelson

pelson Oct 25, 2012

Member

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

This comment has been minimized.

@jenshnielsen

jenshnielsen Oct 25, 2012

Member

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

@jenshnielsen

jenshnielsen Oct 25, 2012

Member

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

lib/matplotlib/tests/test_bbox_tight.py
+import matplotlib.pyplot as plt
+import numpy as np
+
+@image_comparison(baseline_images = ['bbox_inches_tight'], remove_text=True,

This comment has been minimized.

@pelson

pelson Oct 25, 2012

Member

Ideally, we don't have spaces between the kwarg name and it value. (there are a couple of instances of that in this file).

@pelson

pelson Oct 25, 2012

Member

Ideally, we don't have spaces between the kwarg name and it value. (there are a couple of instances of that in this file).

This comment has been minimized.

@jenshnielsen

jenshnielsen Oct 25, 2012

Member

Should be fixed

@jenshnielsen

jenshnielsen Oct 25, 2012

Member

Should be fixed

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Oct 25, 2012

Member

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...

Member

pelson commented Oct 25, 2012

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

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 26, 2012

Member

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?

Member

jenshnielsen commented Oct 26, 2012

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

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 30, 2012

Member

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.

Member

jenshnielsen commented Oct 30, 2012

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

This comment has been minimized.

Show comment
Hide comment
@piti118

piti118 Oct 30, 2012

Contributor

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

Contributor

piti118 commented Oct 30, 2012

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

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Oct 31, 2012

Member

👍 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

Member

pelson commented Oct 31, 2012

👍 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

@ghost ghost assigned pelson Oct 31, 2012

@jenshnielsen

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 31, 2012

Member

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.

Member

jenshnielsen commented Oct 31, 2012

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

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Oct 31, 2012

Member

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,

Member

pelson commented Oct 31, 2012

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

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 31, 2012

Member

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

Member

jenshnielsen commented Oct 31, 2012

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

@jenshnielsen

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 31, 2012

Member

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

Member

jenshnielsen commented Oct 31, 2012

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

@WeatherGod

This comment has been minimized.

Show comment
Hide comment
@WeatherGod

WeatherGod Oct 31, 2012

I am always wary of having mutables for call signature defaults... In this case, we are probably safe, but we might want to double-check on that.

I am always wary of having mutables for call signature defaults... In this case, we are probably safe, but we might want to double-check on that.

This comment has been minimized.

Show comment
Hide comment
@dmcdougall

dmcdougall Oct 31, 2012

We could just pass None instead. Would that be better?

We could just pass None instead. Would that be better?

@jenshnielsen

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Oct 31, 2012

Member

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.

Member

jenshnielsen commented Oct 31, 2012

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

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Nov 1, 2012

Member

@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.

Member

jenshnielsen commented Nov 1, 2012

@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

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Nov 1, 2012

Member

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)?

Member

pelson commented Nov 1, 2012

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

This comment has been minimized.

Show comment
Hide comment
@jenshnielsen

jenshnielsen Nov 1, 2012

Member

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

Member

jenshnielsen commented Nov 1, 2012

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

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Nov 1, 2012

Member

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 .

Member

pelson commented Nov 1, 2012

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 added a commit that referenced this pull request Nov 2, 2012

Merge pull request #1420 from jenshnielsen/bbox_extra_artist
Join bbox_extra_artists and bbox_inches

@pelson pelson merged commit 332ac0a into matplotlib:master Nov 2, 2012

1 check passed

default The Travis build passed
Details
@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Nov 2, 2012

Member

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

Cheers,

Member

pelson commented Nov 2, 2012

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

Cheers,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment