Tick vertical alignment #6200

Merged
merged 2 commits into from Apr 13, 2016

Conversation

Projects
None yet
3 participants
Owner

mdboom commented Mar 21, 2016

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

mdboom added the needs_review label Mar 21, 2016

Owner

tacaswell commented Mar 22, 2016

Seems to have broken all of the pgf tests.

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

Owner

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.

Owner

tacaswell commented Mar 22, 2016

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

Owner

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

Owner

tacaswell commented Mar 22, 2016

fair point.

Owner

tacaswell commented Mar 26, 2016

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

@efiring efiring commented on an outdated diff Mar 27, 2016

lib/matplotlib/text.py
@@ -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)
@efiring

efiring Mar 27, 2016

Owner

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 efiring and 1 other commented on an outdated diff Mar 27, 2016

lib/matplotlib/text.py
@@ -455,6 +457,8 @@ def _get_layout(self, renderer):
offsety = ymax1
elif valign == 'baseline':
offsety = ymax1 - baseline
+ elif valign == 'center_baseline':
+ offsety = (ymin1 + ymax1) / 2.0
@efiring

efiring Mar 27, 2016

Owner

In other words, it is identical to 'center'. Maybe it would be better to just make that explicit by using elif valign in ('center', 'center_baseline'). Would a short comment be enough to explain why this is?

@mdboom

mdboom Apr 11, 2016

Owner

I'm trying to remember why this is -- going to run the tests again and see why...

@mdboom

mdboom Apr 11, 2016

Owner

This was wrong, actually (and not addressed by existing tests, since it's a new vertical alignement type). Fixed now.

Owner

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.

Owner

mdboom commented Apr 11, 2016

I've hopefully addressed @efiring's comments.

Owner

tacaswell commented Apr 11, 2016

@mdboom Now you get to rebase this one 👿

Owner

mdboom commented Apr 11, 2016

Rebased.

Owner

mdboom commented Apr 11, 2016

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

mdboom added some commits Apr 12, 2016

@mdboom mdboom Fix vertical tick alignment
f9d695c
@mdboom mdboom Increase test tolerance
6b9ad78
Owner

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

efiring removed the needs_review label Apr 13, 2016

Owner

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 mdboom added a commit to mdboom/matplotlib that referenced this pull request Apr 13, 2016

@efiring @mdboom efiring + mdboom Merge pull request #6200 from mdboom/tick-vertical-alignment
Tick vertical alignment
5fb9d9e
Owner

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