8000 FIX: don't include text at -inf in bbox by jklymak · Pull Request #11404 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

FIX: don't include text at -inf in bbox #11404

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 3, 2018
Merged

FIX: don't include text at -inf in bbox #11404

merged 1 commit into from
Jul 3, 2018

Conversation

jklymak
Copy link
Member
@jklymak jklymak commented Jun 9, 2018 •
edited
Loading

PR Summary

Adresses #11386, #5456. See also #11417 that properly fixes the logit issues...

Code like below caused anything that required the tightbbox of the axes to fail forlogit axes:

This PR does not fix logit axes, but instead ignores non-finite values of the text positions when calculating the bbox, since those are assumed to be invalid. I suppose someone could also figure out how to fix logit, but I'm not too interested in digging into it.

import matplotlib.pyplot as plt
import numpy as np
fig, ax = plt.subplots()

np.random.seed(1)
y = np.random.normal(loc=0.5, scale=0.4, size=1000)
y = y[(y > 0) & (y < 1)]
y.sort()
x = np.arange(len(y))
ax.plot(x, y)
ax.set_yscale('logit')
print(ax.get_tightbbox(fig.canvas.get_renderer()))

Before

/Users/jklymak/matplotlib/lib/matplotlib/transforms.py:402: RuntimeWarning: invalid value encountered in double_scalars
  return points[1, 0] - points[0, 0]
Bbox(x0=-inf, y0=nan, x1=1155.5, y1=nan)

After

Bbox(x0=-21.6925048828125, y0=58.15555555555555, x1=1155.5, y1=848.3)

PR Checklist

    < 8000 li class="task-list-item"> Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak mentioned this pull request Jun 10, 2018
Closed
anntzer
anntzer reviewed Jun 10, 2018
View reviewed changes
lib/matplotlib/axis.py Outdated
bb.append(a.get_window_extent(renderer))
x, y = a.get_position()
if np.isfinite(x) and np.isfinite(y):
bb.append(a.get_window_extent(renderer))
Copy link
Contributor

Choose a reason for hiding this comment

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

Spent ~2s thinking about this but in general wouldn't it be better to first get the bbox and then include it only if it is finite? Or does the exception occur in get_window_extent? (in which case I'd argue it should be fixed there)

In fact perhaps get_window_extend should return a null bbox (do we even have a way to represent that) in that case, to avoid having to special-case it again and again. Or perhaps not (again, just throwing in some comments).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

However, I don't think the API allows returning a None for the window extent, so we'll have to check the dims of the bbox are reasonable w/o an API change

@tacaswell tacaswell added this to the v3.0 milestone Jun 10, 2018
@jklymak jklymak force-pushed the fix-inf-text branch 2 times, most recently from 4fd7998 to 9326553 Compare June 10, 2018 15:22
@jklymak
Copy link
Member Author
jklymak commented Jun 10, 2018

There is still something wrong with the logit axis transform that causes _update_label_position to get put at infinity as @frederik-1 said. Setting the label for logit axis never works which is really the bug. Nonetheless this symptom patch is still probably a good idea.

@jklymak jklymak mentioned this pull request Jun 11, 2018
Merged
6 tasks
@efiring
Copy link
Member
efiring commented Jul 3, 2018

With master I am not able to reproduce the problem using the test case you provide:

(test) ~/work/programs/py/mpl/tests $ python issue11404.py
Bbox(x0=-21.6925048828125, y0=58.15555555555555, x1=1155.5, y1=848.3)

Do we still need this PR? Or is the original problem fixed by #11417?

@jklymak
Copy link
Member Author
jklymak commented Jul 3, 2018

We don’t need it to fix the proximate problem anymore but it can’t hurt to check the box has width and height before appending it.

@efiring efiring merged commit 4d7f8ce into matplotlib:master Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer left review comments

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jklymak @efiring @anntzer @timhoffm @tacaswell
0