test case for R<-1 in scipy.stats.linregress and fix in the code #433

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@kif
Contributor

kif commented Feb 12, 2013

Code for preventing R to be lower than -1.0 due to accumulation of numerical error scipy.stats.linregress and a test-case (tested under debian7, python2.7, amd64 and probably dependent on the compiler as well)

scipy/stats/tests/test_stats.py
+ y = np.linspace(2 * a, a, n)
+ stats.linregress(x, y)
+ res = stats.linregress(x, y)
+ assert_(res[2] >= -1, "R factor is in [-1..1]")

This comment has been minimized.

@josef-pkt

josef-pkt Feb 12, 2013

Member

the assertion is too loose, shouldn't this be
assert_equal(res[2], -1, ...)
there is a small chance that equality doesn't hold for all platforms
the we could use both (just add an almost equal)

assert_(res[2] >= -1, "R factor is in [-1..1]")
assert_almost_equal(res[2]), -1)
@josef-pkt

josef-pkt Feb 12, 2013

Member

the assertion is too loose, shouldn't this be
assert_equal(res[2], -1, ...)
there is a small chance that equality doesn't hold for all platforms
the we could use both (just add an almost equal)

assert_(res[2] >= -1, "R factor is in [-1..1]")
assert_almost_equal(res[2]), -1)
scipy/stats/stats.py
@@ -2962,6 +2962,7 @@ def linregress(x, y=None):
else:
r = r_num / r_den
if (r > 1.0): r = 1.0 # from numerical error
+ elif (r < -1.0): r = -1.0 # from numerical error

This comment has been minimized.

@josef-pkt

josef-pkt Feb 12, 2013

Member

cosmetic: move r = ... into new lines (also the old)

@josef-pkt

josef-pkt Feb 12, 2013

Member

cosmetic: move r = ... into new lines (also the old)

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Feb 12, 2013

Member

@rgommers should go into 0.12

Member

josef-pkt commented Feb 12, 2013

@rgommers should go into 0.12

@kif

This comment has been minimized.

Show comment
Hide comment
@kif

kif Feb 12, 2013

Contributor

I think all your comment were taken into account. Josef, is it OK ?

Contributor

kif commented Feb 12, 2013

I think all your comment were taken into account. Josef, is it OK ?

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Feb 12, 2013

error message could be improved, it's purpose is mainly to get the information about the test in case of failure
maybe just something like "perfect negative correlation case"

error message could be improved, it's purpose is mainly to get the information about the test in case of failure
maybe just something like "perfect negative correlation case"

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Feb 12, 2013

just additional info:
The numbers, actual and expected, are shown automatically in case of a test failure, so it doesn't need to be in the error message.

just additional info:
The numbers, actual and expected, are shown automatically in case of a test failure, so it doesn't need to be in the error message.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Feb 14, 2013

Member

Squashed and merged as c415f8d. Make a minor change to the messages - they're comments now. Failure will then show the actual numerical values.

Thanks Jerome, Josef.

Member

rgommers commented Feb 14, 2013

Squashed and merged as c415f8d. Make a minor change to the messages - they're comments now. Failure will then show the actual numerical values.

Thanks Jerome, Josef.

@rgommers rgommers closed this Feb 14, 2013

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