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
Update tight_bbox.py [backport to 1.4.x] #3434
Conversation
Typo in the figure parameter
Good catch! I guess this particular function isn't exercised somehow. Looking through git blame, it appears that @pwuertz changed "figure" to "fig" in the call signature back in 2013. Any way to add a test for this function? |
Well, I found it because it broke my code. Here is the example:
|
Ops, my fault. I'm wondering though why that error didn't pop up within the unit tests. Especially since rasterization was the one thing I extensively tested back then :/. |
I guess the right thing to do is to add a test to test_bbox_tight.py with @jowr Are you willing to do that? |
Hi, I had a look a the tests, but could not figure out how to use the existing infrastructure properly. I prefer not to add the tests myself and reckon that it is going to be little work for a person familiar with the matplotlib testing facilities. |
@jowr we are happy to help guide you in the right direction if you wish. Alternatively I am happy to do the test and submit a pull request to you pull request. |
Thanks @jenshnielsen, let me try to describe my confusion and please correct me where necessary. I think it all boils down to: How is the normal testing workflow? Here is what I did: First, I cloned my forked repo and switched to the branch if this pull request The second question is: What should the tests cover? Is it enough to call the function and make sure that no exceptions occur or should we have an image to compare the output to? |
I forgot to mention my environment:
|
The tests run on the installed version. If you are doing a lot of development, virtual environments are really helpful. In this case I think just calling the function should be sufficient. |
Concerning image tests. Yes I think we should make this an image comparison test test. The particular You want to use the @image_comparison(baseline_images=['bbox_inches_tight_raster'],
remove_text=True, savefig_kwarg={'bbox_inches': 'tight'})
def test_bbox_inches_tight_raster():
"code to produce plot here ..."
Modified from one of the existing tests in the file. Then baseline_images indicate the file name of the images that this is compared with. These should go into |
Thanks for all the help, but it seems like I was missing something obvious. I installed my initial version via pip
Changing the permissions with Besides that, I managed to include the tests and pushed the commit. I hope that I am better prepared for next time now 😉 . A remaining issue is that the SVG comparison fails with One more thing: With the current version 1.5.x, the code I posted above does not produce any errors. This seems to be a problem with 1.4.x only. I am not sure how to proceed since this fix is not needed for 1.5.x to work. |
The install permissions sounds like a bug. Could you create a separate issue for that? You need inkscape to compare svg's (convert to png actually) this is most likely the reason. The message could be more informative. I am still seeing the issue when using the alternative method for tight_layout saving with Could you copy the test to this file and use an decorator like the following (modifying the name as you like) @image_comparison(baseline_images=['bbox_inches_tight_raster'],
remove_text=True, savefig_kwarg={'bbox_inches': 'tight'}) this pases bbox_inches='tight' to fig.savefig such that it is called as: plt.savefig("filename.pdf", bbox_inches='tight') On master this produces ---------------------------------------------------------------------------
NameError Traceback (most recent call last)
<ipython-input-14-98f93d09d7a6> in <module>()
----> 1 plt.savefig("fix_pull-3434.pdf", bbox_inches='tight')
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/pyplot.pyc in savefig(*args, **kwargs)
574 def savefig(*args, **kwargs):
575 fig = gcf()
--> 576 res = fig.savefig(*args, **kwargs)
577 draw() # need this if 'transparent=True' to reset colors
578 return res
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/figure.pyc in savefig(self, *args, **kwargs)
1468 self.set_frameon(frameon)
1469
-> 1470 self.canvas.print_figure(*args, **kwargs)
1471
1472 if frameon:
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/backend_bases.pyc in print_figure(self, filename, dpi, facecolor, edgecolor, orientation, format, **kwargs)
2190 orientation=orientation,
2191 bbox_inches_restore=_bbox_inches_restore,
-> 2192 **kwargs)
2193 finally:
2194 if bbox_inches and restore_bbox:
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/backends/backend_pdf.pyc in print_pdf(self, filename, **kwargs)
2467 RendererPdf(file, image_dpi),
2468 bbox_inches_restore=_bbox_inches_restore)
-> 2469 self.figure.draw(renderer)
2470 renderer.finalize()
2471 finally:
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/artist.pyc in draw_wrapper(artist, renderer, *args, **kwargs)
57 def draw_wrapper(artist, renderer, *args, **kwargs):
58 before(artist, renderer)
---> 59 draw(artist, renderer, *args, **kwargs)
60 after(artist, renderer)
61
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/figure.pyc in draw(self, renderer)
1077 dsu.sort(key=itemgetter(0))
1078 for zorder, a, func, args in dsu:
-> 1079 func(*args)
1080
1081 renderer.close_group('figure')
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/artist.pyc in draw_wrapper(artist, renderer, *args, **kwargs)
57 def draw_wrapper(artist, renderer, *args, **kwargs):
58 before(artist, renderer)
---> 59 draw(artist, renderer, *args, **kwargs)
60 after(artist, renderer)
61
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/axes/_base.pyc in draw(self, renderer, inframe)
2090
2091 for zorder, a in dsu:
-> 2092 a.draw(renderer)
2093
2094 renderer.close_group('axes')
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/artist.pyc in draw_wrapper(artist, renderer, *args, **kwargs)
56 # the axes class has a second argument inframe for its draw method.
57 def draw_wrapper(artist, renderer, *args, **kwargs):
---> 58 before(artist, renderer)
59 draw(artist, renderer, *args, **kwargs)
60 after(artist, renderer)
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/artist.pyc in before(artist, renderer)
41 def before(artist, renderer):
42 if artist.get_rasterized():
---> 43 renderer.start_rasterizing()
44
45 if artist.get_agg_filter() is not None:
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/backends/backend_mixed.pyc in start_rasterizing(self)
94 if self._bbox_inches_restore: # when tight bbox is used
95 r = process_figure_for_rasterizing(self.figure,
---> 96 self._bbox_inches_restore)
97 self._bbox_inches_restore = r
98
/usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-macosx-10.9-x86_64.egg/matplotlib/tight_bbox.pyc in process_figure_for_rasterizing(fig, bbox_inches_restore, fixed_dpi)
84 bbox_inches, restore_bbox = bbox_inches_restore
85 restore_bbox()
---> 86 r = adjust_bbox(figure, bbox_inches, fixed_dpi)
87
88 return bbox_inches, r
NameError: global name 'figure' is not defined for me. |
Our CI system Travis will automatically run the tests even if you can't run it |
@image_comparison(baseline_images=['tight_layout8']) | ||
def test_tight_layout8(): | ||
'Test automatic use of tight_layout' | ||
print("Hello") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the debugging print statement?
I hope that you can use this PR now... |
Looks good to me. Will just wait for travis to run the tests. |
Hm, I do not know why it fails on Travis. I think all tests pass locally. I this related to the SVG issue?
|
There might be a slight difference between saving through saving using the On Tue, Sep 2, 2014 at 11:00 AM, Jorrit Wronski notifications@github.com
|
I copied them from the result directory
|
Does the error message mean that the images differ by column of pixels? |
Yes for some reason the png generated from your svg is different in size by one. I have made a pull request with the version that I have generated against your branch. If you merge it the test should pass. Sorry for all the trouble this has given. |
I guess this is due to some small difference in some library on your computer. |
New version of SVG file
We are running a pep8 style test on the code base and before your added line this was failing on test_tight_layout.py. Adding the line fixed this and made the file conform to the style guide. However, this causes the test to fail because a file that was marked as non conforming was conforming. This could have been fixed by removing the file from EXPECTED_BAD_FILES in test_coding_standards But no need to fix this now since your tests are not touching that file any more. |
Thanks for the explanation, it seems like a whole machinery waits around every corner 😃 . |
Sorry that this turned out to be so complicated. Hopefully it will not discourage you from contributing again. |
I am going to merge this via #3452 which contains the same changes but is rebased to remove the additional svg file. We try to avoid to many versions of test images to reduce the growth of the repository size. |
Make that #3453 since I opened the other one one the wrong branch. |
Closing since the rebase was merged |
Thanks for all the help. I do not feel discouraged... |
@jowr Thanks again for finding and fixing this! |
@tacaswell : Welcome! |
Typo in the figure parameter