Skip to content

Fix #5456: Keep margins <= .5 in tight_layout #6161

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

Closed
wants to merge 4 commits into from

Conversation

josecv
Copy link

@josecv josecv commented Mar 15, 2016

For #5456 added a check to keep margins < .5 to prevent an Exception



def test_long_label():
'''
Copy link
Member

Choose a reason for hiding this comment

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

Please use 4-space indent.

@josecv
Copy link
Author

josecv commented Mar 15, 2016

Ok, thanks, does this look better?

@josecv
Copy link
Author

josecv commented Mar 16, 2016

Hurm the tests seem to be failing with a latex error on windows, could it be the ylabel is too long for latex' tastes?

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Mar 16, 2016
@mdboom
Copy link
Member

mdboom commented Mar 16, 2016

It seems like the test failure is unrelated. Has anyone seen that before? (Cc: @JanSchulz)

@jankatins
Copy link
Contributor

It seems latex is run in that test (if that's not ok, then the rest of my explanation is wrong :-)), but I'm not sure if appveyor has latex installed (I'm pretty sure it has not). Usually there are test decorators which test for latex and skip the test if no latex is available on the system: https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_backend_ps.py#L22-L25 and used then in https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_backend_ps.py#L72-L78

====================================================================== 
ERROR: Test for 5456. When the label was too long we'd get a ValueError that 
---------------------------------------------------------------------- 
Traceback (most recent call last):
  File "C:\conda\envs\test-environment\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "c:\projects\matplotlib\lib\matplotlib\tests\test_tightlayout.py", line 266, in test_long_label
    plt.tight_layout()
  File "c:\projects\matplotlib\lib\matplotlib\pyplot.py", line 1282, in tight_layout
    fig.tight_layout(pad=pad, h_pad=h_pad, w_pad=w_pad, rect=rect)
  File "c:\projects\matplotlib\lib\matplotlib\figure.py", line 1857, in tight_layout
    rect=rect)
  File "c:\projects\matplotlib\lib\matplotlib\tight_layout.py", line 355, in get_tight_layout_figure
    pad=pad, h_pad=h_pad, w_pad=w_pad)
  File "c:\projects\matplotlib\lib\matplotlib\tight_layout.py", line 126, in auto_adjust_subplotpars
    tight_bbox_raw = union([ax.get_tightbbox(renderer) for ax in subplots])
  File "c:\projects\matplotlib\lib\matplotlib\axes\_base.py", line 3750, in get_tightbbox
    bb_xaxis = self.xaxis.get_tightbbox(renderer)
  File "c:\projects\matplotlib\lib\matplotlib\axis.py", line 1087, in get_tightbbox
    renderer)
  File "c:\projects\matplotlib\lib\matplotlib\axis.py", line 1070, in _get_tick_bboxes
    extent = tick.label1.get_window_extent(renderer)
  File "c:\projects\matplotlib\lib\matplotlib\text.py", line 961, in get_window_extent
    bbox, info, descent = self._get_layout(self._renderer)
  File "c:\projects\matplotlib\lib\matplotlib\text.py", line 352, in _get_layout
    ismath=False)
  File "c:\projects\matplotlib\lib\matplotlib\backends\backend_agg.py", line 230, in get_text_width_height_descent
    renderer=self)
  File "c:\projects\matplotlib\lib\matplotlib\texmanager.py", line 676, in get_text_width_height_descent
    dvifile = self.make_dvi(tex, fontsize)
  File "c:\projects\matplotlib\lib\matplotlib\texmanager.py", line 423, in make_dvi
    report))
RuntimeError: LaTeX was not able to process the following string:
'lp'
Here is the full report generated by LaTeX: 

@mdboom
Copy link
Member

mdboom commented Mar 16, 2016

Ah -- thanks, @JanSchulz. I don't LaTeX is supposed to be used in that test, but due to some sideeffect (probably of another test) it is.

@josecv: Can you try adding the @cleanup decorator to your new test?

@jankatins
Copy link
Contributor

Cleanup is not enough, it has to skip if no Latex is installed.

@josecv
Copy link
Author

josecv commented Mar 16, 2016

Ah, perfect, thanks! Hopefully this should do the trick

@mdboom
Copy link
Member

mdboom commented Mar 16, 2016

Ok -- I think the confusion is that the new test in this PR is not responsible for the failure, but it somehow triggering it in another test -- test_backend_svg:test_determinism_tex, which already has the @needs_tex decorator.

It isn't clear to me why the next test here needs to check for TeX -- it shouldn't be using it and isn't part of the traceback displayed on AppVeyor.

@jankatins
Copy link
Contributor

I've seens such behaviour when I missed a @cleanup in the tests of ggplot, but this is here: https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_backend_svg.py#L179-L184 -> I've no idea :-(

@josecv
Copy link
Author

josecv commented Mar 16, 2016

Huh Known Failure Did Not Fail:

====================================================================== 
ERROR: Test for 5456. No ValueError when using very long label. 
---------------------------------------------------------------------- 
Traceback (most recent call last):
  File "C:\conda\envs\test-environment\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 152, in wrapped_callable
    func(*args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 67, in failer
    raise KnownFailureDidNotFailTest(msg)
KnownFailureDidNotFailTest: This test needs a Tex installation  
----------------------------------------------------------------------

Is it possible that the cleanup decorator on its own is allowing the test to pass?

@WeatherGod
Copy link
Member

Well, more specifically, it did fail, but for the wrong reasons.

I am wondering if usetex=True is leaking from somewhere else?

On Wed, Mar 16, 2016 at 1:08 PM, josecv notifications@github.com wrote:

Huh Known Failure Did Not Fail:

ERROR: Test for 5456. No ValueError when using very long label.

Traceback (most recent call last):
File "C:\conda\envs\test-environment\lib\site-packages\nose\case.py", line 197, in runTest
self.test(_self.arg)
File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 152, in wrapped_callable
func(_args, **kwargs)
File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 67, in failer
raise KnownFailureDidNotFailTest(msg)

KnownFailureDidNotFailTest: This test needs a Tex installation

Is it possible that the cleanup decorator on its own is allowing the test
to pass?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#6161 (comment)

@josecv
Copy link
Author

josecv commented Mar 18, 2016

Hmm, strange - hopefully then leaving the test with only @cleanup is going to do the trick

@josecv
Copy link
Author

josecv commented Mar 18, 2016

Oh my, bizarrely it's the Travis build that's failed now, though it seems to be an unrelated failure in a single test (namely TEST_ARGS=--pep8 with python 3.5):

The command "curl -s -o python-3.5.tar.bz2 https://s3.amazonaws.com/travis-python-archives/binaries/$(lsb_release -is | tr "A-Z" "a-z")/$(lsb_release -rs)/$(uname -m)/python-3.5.tar.bz2" failed and exited with 18 during .

Your build has been stopped.

@WeatherGod
Copy link
Member

And now everything passes. Go figure.

Just one thought on this. This approach solves the problem for the fully automatic case, but what about the case where the user specified some of the margins?

@josecv
Copy link
Author

josecv commented Mar 18, 2016

That's a good point - the min statements could easily be pulled out from the if not margin_*: statements, would that be the best thing to do?
The user would find that their margins are being ignored, but at least there won't be an exception. Alternatively, tight_layout could itself produce an error when fed "bad" (out of bounds) margins.

@tacaswell
Copy link
Member

#6167 is another PR addressing the same issue. I left some comments on the other PR that I think apply here as well.

@kskod @josecv Are you both in CSC D01? If so can you please collaborate on this?

@QuLogic QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017
@anntzer
Copy link
Contributor

anntzer commented May 13, 2017

I am closing this as I believe we should instead have a more general solution which raises whenever the padding/spacing parameters make tight-layouting impossible (#8062, #8201, #8202) (or handles correctly such cases).
Feel free to ask for a reopen if you disagree.

@anntzer anntzer closed this May 13, 2017
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.

7 participants