Skip to content

Tick vertical alignment #6200

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

Merged
merged 2 commits into from
Apr 13, 2016
Merged

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Mar 21, 2016

Fixes #2527. Align y-ticks without accounting for descenders

@mdboom mdboom added this to the 2.0 (style change major release) milestone Mar 21, 2016
@tacaswell
Copy link
Member

Seems to have broken all of the pgf tests.

That makes me a bit worried about other backends that we do not control....

@mdboom
Copy link
Member Author

mdboom commented Mar 22, 2016

Sorry -- I have a new box and I think I don't have all the pgf reqs installed yet. I can probably regenerate those.

As for other backends we don't control, I think we'll just be seeing better tick text alignment in all of them -- this approach is delibrately rather narrow and shouldn't affect other kinds of text. But, yes, we will be breaking those other backends image comparison tests, if any.

@tacaswell
Copy link
Member

======================================================================
ERROR: matplotlib.tests.test_backend_pgf.test_bbox_inches
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 422, in backend_switcher
    result = func(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_backend_pgf.py", line 193, in test_bbox_inches
    tol=0)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_backend_pgf.py", line 46, in compare_figure
    plt.savefig(actual, **savefig_kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/pyplot.py", line 686, in savefig
    res = fig.savefig(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/figure.py", line 1666, in savefig
    self.canvas.print_figure(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backend_bases.py", line 2232, in print_figure
    **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backends/backend_pgf.py", line 894, in print_pdf
    self._print_pdf_to_fh(fh, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backends/backend_pgf.py", line 848, in _print_pdf_to_fh
    self.print_pgf(fname_pgf, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backends/backend_pgf.py", line 830, in print_pgf
    self._print_pgf_to_fh(fh, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backends/backend_pgf.py", line 811, in _print_pgf_to_fh
    self.figure.draw(renderer)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/artist.py", line 63, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/figure.py", line 1262, in draw
    renderer, self, dsu, self.suppressComposite)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/image.py", line 139, in _draw_list_compositing_images
    a.draw(renderer)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/artist.py", line 63, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_base.py", line 2355, in draw
    mimage._draw_list_compositing_images(renderer, self, dsu)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/image.py", line 139, in _draw_list_compositing_images
    a.draw(renderer)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/artist.py", line 63, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axis.py", line 1133, in draw
    tick.draw(renderer)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/artist.py", line 63, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axis.py", line 268, in draw
    self.label1.draw(renderer)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/artist.py", line 63, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/text.py", line 800, in draw
    ismath=ismath, mtext=mtext)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/backends/backend_pgf.py", line 667, in draw_text
    text_args.append(valign[mtext.get_va()])
KeyError: 'center_baseline'

This looks like the render is breaking.

@mdboom
Copy link
Member Author

mdboom commented Mar 22, 2016

Oh, and I should have read the error message first. It seems that PGF, unlike all other backends in the core, handles the text alignment itself -- or, more specifically, delgates it to the TeX engine. That seems like a corner case to me that most third-party backends won't face, but point taken that it could break them. However, we are bumping the major version number for the first time in years, if not now, when? :)

@tacaswell
Copy link
Member

fair point.

@tacaswell
Copy link
Member

I am 👍 on this, but given how many tests it touches someone else should agree as well.

@@ -436,6 +436,8 @@ def _get_layout(self, renderer):
offsety = (ymin + height)
elif valign == 'baseline':
offsety = (ymin + height) - baseline
elif valign == 'center_baseline':
offsety = (ymin + (height + (height - baseline)) / 2.0)
Copy link
Member

Choose a reason for hiding this comment

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

In other words, ymin + height - baseline / 2. This is the same as 'top' minus baseline / 2 which makes sense, since baseline is the height starting from the baseline rather than the descender. Right?

@efiring
Copy link
Member

efiring commented Mar 27, 2016

I think it is OK. The relevant part of the text code was already not so easy to understand, so I made a couple of suggestions for comments and simplification. I don't understand why the anchored rotation case is handled as it is.

@mdboom
Copy link
Member Author

mdboom commented Apr 11, 2016

I've hopefully addressed @efiring's comments.

@tacaswell
Copy link
Member

@mdboom Now you get to rebase this one 👿

@mdboom mdboom force-pushed the tick-vertical-alignment branch from 7293734 to 697d8e1 Compare April 11, 2016 22:29
@mdboom
Copy link
Member Author

mdboom commented Apr 11, 2016

Rebased.

@mdboom
Copy link
Member Author

mdboom commented Apr 11, 2016

I'll need to rerun tests and squash -- stay tuned...

@mdboom mdboom force-pushed the tick-vertical-alignment branch 3 times, most recently from c967de5 to 7e4d26a Compare April 12, 2016 12:50
@mdboom mdboom force-pushed the tick-vertical-alignment branch from c434aa1 to f9d695c Compare April 12, 2016 13:08
@mdboom
Copy link
Member Author

mdboom commented Apr 13, 2016

Passsing the tests! Let's merge this before I have to rebase and rebuild all the tests again :)

@efiring efiring merged commit 6b22bfc into matplotlib:master Apr 13, 2016
@mdboom
Copy link
Member Author

mdboom commented Apr 13, 2016

I'm backporting this to 2.x now, but there's some conflicts, so it might take a while...

mdboom pushed a commit to mdboom/matplotlib that referenced this pull request Apr 13, 2016
@mdboom
Copy link
Member Author

mdboom commented Apr 13, 2016

Backported to 2.x as 5fb9d9e

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.

3 participants