BUG: smirnov -- make comparison robust to handle NaNs #187

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

yarikoptic commented Mar 31, 2012

any comparison (< or >=) with NaN is False, so check to belong to the range
[x,y] must be done with (>=x)&&(<=y) and not !((<x)||(>y))

This issue happens to be critical especially for big-endian architectures
leading to stalling in smirnov due to excessive looping later in the code.

References:
http://bugs.debian.org/cgi-bin/bugreport.cgi\?bug\=653948
http://mail.scipy.org/pipermail/scipy-dev/2012-March/017298.html

@yarikoptic yarikoptic BUG: smirnov -- make comparison robust to handle NaNs
any comparison (< or >=) with NaN is False, so check to belong to the range
[x,y] must be done with (>=x)&&(<=y) and not !((<x)||(>y))

This issue happens to be critical especially for big-endian architectures
leading to stalling in smirnov due to excessive looping later in the code.

References:
  http://bugs.debian.org/cgi-bin/bugreport.cgi\?bug\=653948
  http://mail.scipy.org/pipermail/scipy-dev/2012-March/017298.html
ebe8fbe
Owner

pv commented Mar 31, 2012

Thanks, this looks correct. A small regression test should be added (e.g. during merge).

Bug ref: http://projects.scipy.org/scipy/ticket/1638

Contributor

yarikoptic commented Apr 2, 2012

Just to make clear -- a regression test was not expected from me, was it? ;-)

Owner

pv commented Apr 2, 2012

@yarikoptic: Well, if you add it, someone else doesn't need to :) But if you don't, it shouldn't hold back merging this

Contributor

yarikoptic commented May 9, 2012

tested that it does halt with unpatched version and completes with patched one (not sure if that qualifies for a term "regression test" without comparing to smth :-P ;-) ) Thank you @stefanv !

Owner

stefanv commented May 9, 2012

It will catch the regression, at least :) @pv what do you think?

Owner

pv commented May 11, 2012

Merged in f7083b6

pv closed this May 11, 2012

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