Fixed potential overflow exception in the lines.contains() method #1002

Merged
merged 1 commit into from Jul 21, 2012

Conversation

Projects
None yet
3 participants
Contributor

dhyams commented Jul 10, 2012

Fixed potential overflow exception in the lines.contains() method, if the application happened to have set the numpy error flags to raise exceptions in case of overflow.

@dhyams dhyams Fixed potential overflow exception in the lines.contains() method, if…
… the application happened

to have set the numpy error flags to raise exceptions in case of overflow.
6f3dab5
Member

pelson commented Jul 18, 2012

Ok. I have no problem with the changes you have proposed, but I am interested how this manifests itself. Knowing this information, is it possible to pinpoint the problem to a single line or do both cases of the if statement need to be handled?

Contributor

dhyams commented Jul 18, 2012

It manifests when you have a function that has values of some huge numbers
(as can happen when you evaluate, say 1/x near zero), and if the
application has set the numpy error flags such that exceptions will be
raised on overflow.

It's the squaring of (xt-mouseevent.x) that trips the overflow exception if
the above is true.

The patch basically sets and restores the numpy error flags just during the
hit testing calculation, so that overflows are OK. It seems that
(understandably so) that matplotlib doesn't care much about overflow,
because the default numpy error flags are

{'over': 'warn', 'divide': 'warn', 'invalid': 'warn', 'under': 'ignore'}

and that's the environment that mpl operates under nearly all of the time.

I might be going off on a rabbit trail here, but in my opinion, the nose
tests should be run with more strict numpy numerical treatment:

{'over': 'raise', 'divide': 'raise', 'invalid': 'raise', 'under': 'ignore'}

I tried to get in there and do this myself to see what the results are, but
it looked like I would have to decorate every single test function in
matplotlib.tests. I might ask on the user group and/or try again later.

On Wed, Jul 18, 2012 at 2:33 PM, Phil Elson <
reply@reply.github.com

wrote:

Ok. I have no problem with the changes you have proposed, but I am
interested how this manifests itself. Knowing this information, is it
possible to pinpoint the problem to a single line or do both cases of the
if statement need to be handled?


Reply to this email directly or view it on GitHub:
#1002 (comment)

Daniel Hyams
dhyams@gmail.com

Contributor

dhyams commented Jul 18, 2012

Oh, and sorry, to answer your question about the two branches, yes both need to be covered; both involve squaring (xt-mouseevent.x) and (yt-mouseevent.y)

Member

WeatherGod commented Jul 21, 2012

This looks good to me. I see no reason not to merge. Thanks for this!

@WeatherGod WeatherGod added a commit that referenced this pull request Jul 21, 2012

@WeatherGod WeatherGod Merge pull request #1002 from dhyams/fix_segment_hits
Fixed potential overflow exception in the lines.contains() method
be9a310

@WeatherGod WeatherGod merged commit be9a310 into matplotlib:master Jul 21, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment