8000 Nan issue by ffteja · Pull Request #4235 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Nan issue #4235

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
Closed
wants to merge 3 commits into from
Closed

Nan issue #4235

wants to merge 3 commits into from

Conversation

ffteja
Copy link
Contributor
@ffteja ffteja commented Mar 17, 2015

PR for issue #4066

@pelson
Copy link
Member
pelson commented Mar 17, 2015

👍

tacaswell
tacaswell reviewed Mar 17, 2015
View reviewed changes
lib/matplotlib/text.py
x = x + posx
y = y + posy
if renderer.flipy():
y = canvash - y
if not np.isfinite(x) or not np.isfinite(y):
continue

Copy link
Member

Choose a reason for hiding this comment

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

whitespace in this line

@tacaswell
Copy link
Member

You have trailing whitespace in two of the blank lines which is what is making travis fail.

@tacaswell tacaswell added this to the next point release milestone Mar 17, 2015
@mdboom
Copy link
Member
mdboom commented Mar 17, 2015

👍

@mdboom
Copy link
Member
mdboom commented Mar 17, 2015

Hmm... if the mathtext points contain non-finite values, I'd consider that a bug in the mathtext renderer. posx and posy could, of course, be non-finite for a number of reasons. So I would take this check outside of the inner loop and check for posx or posy being non-finite.

tacaswell
tacaswell reviewed Mar 20, 2015
View reviewed changes
lib/matplotlib/text.py
x = x + posx
y = y + posy
if renderer.flipy():
y = canvash - y
if not np.isfinite(x) or not np.isfinite(y):
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this check up to just after line 610?

@tacaswell
Copy link
Member

@ffteja Can you make the requested change?

@efiring
Copy link
Member
efiring commented May 14, 2015

It looks like this is functionally ready to go. It would be nice to have it squashed down to a single commit. A test would also be nice, if a simple one can be constructed.

@tacaswell
Copy link
Member

This needs a re-base (due to the text wrapping PR).

I am not sure we want to promote this to an exception rather than just not drawing.

@tacaswell
Copy link
Member

@ffteja Can you rebase this?

@tacaswell tacaswell mentioned this pull request Jun 17, 2015
Merged
@tacaswell
Copy link
Member

@ffteja I have done the re-base + squash and opened a new PR (#4535)

@tacaswell tacaswell closed this Jun 17, 2015
@tacaswell tacaswell mentioned this pull request Oct 5, 2017
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ffteja @pelson @tacaswell @mdboom @efiring
0