rjbs / Number-Tolerant

perl library for numbers with built-in tolerances
3 stars 4 forks source link

spaceship doesn't DWIM #2

Closed karenetheridge closed 8 years ago

karenetheridge commented 10 years ago

It appears this is a bug?

diff --git a/t/plusminus.t b/t/plusminus.t
index ec6abe5..e424849 100644
--- a/t/plusminus.t
+++ b/t/plusminus.t
@@ -1,4 +1,4 @@
-use Test::More tests => 48;
+use Test::More;

 use strict;
 use warnings;
@@ -37,6 +37,15 @@ is( (4 <=> $guess), -1,   " ... 4 <=> it is -1");
 is( (5 <=> $guess),  0,   " ... 5 <=> it is  0");
 is( (6 <=> $guess), +1,   " ... 6 <=> it is +1");

+is( (4.4 <=> $guess), -1,  " ... 4.4 is less than it");
+is( (4.5 <=> $guess),  0,  " ... 4.5 is equal to it");
+is( (5.0 <=> $guess),  0,  " ... 5.0 is equal to it");
+is( (5.5 <=> $guess),  0,  " ... 5.5 is equal to it");
+is( (5.6 <=> $guess),  1,  " ... 5.6 is more than it");
+
 # ... and now more of the same, BACKWARDS

 ok($guess != 0.0,         " ... it isn't equal to 0.0");
@@ -74,3 +80,5 @@ is( ($guess <=> 6), -1,   " ... 6 <=> it is +1");
   isa_ok($tol, 'Number::Tolerant');
        is($tol, "10 +/- 2", "plus_or_minus");
 }
+
+done_testing;
karenetheridge commented 10 years ago
ok 25 -  ... 4.4 is less than it
not ok 26 -  ... 4.5 is equal to it
#   Failed test ' ... 4.5 is equal to it'
#   at t/plusminus.t line 41.
#          got: '-1'
#     expected: '0'
ok 27 -  ... 5.0 is equal to it
not ok 28 -  ... 5.5 is equal to it
#   Failed test ' ... 5.5 is equal to it'
#   at t/plusminus.t line 43.
#          got: '1'
#     expected: '0'
ok 29 -  ... 5.6 is more than it
rjbs commented 10 years ago

Fixing this bug causes a lot of failures. I think that in all cases, the tests are wrong. They're a mess.

Here's the change I made in the Number::Tolerant overloads:

   '<=>' => sub {
-    $_[2] ? ($_[1] <=> $_[0]->{value}) : ($_[0]->{value} <=> $_[1])
+    my $rv = $_[0] == $_[1] ? 0
+           : $_[0] <  $_[1] ? -1
+           : $_[0] >  $_[1] ? 1
+           : die "impossible";
+    $rv *= -1 if $_[2];
+    return $rv;

The old code only compared midpoints and I'm not sure I understand why. Blame gets me back to the initial first commit in 2005. I think that my concern was something about non-continuous ranges, but I think the semantics seem clear: <=> can and should be defined in terms of the other comparitors.

Then there are tests like this:

is( (4    <=> $guess), -1, " ... 4    <=> it is -1");                             
is( (4.75 <=> $guess),  0, " ... 4.75 <=> it is 0");                              
is( (5    <=> $guess), +1, " ... 5    <=> it is +1");                             
is( (6    <=> $guess), +1, " ... 6    <=> it is +1");                             

The comments are backward (it's doing $x <=> 6 but the comment is 6 <=> it). The range here is effectively 4.5-to-5.5, so $x<=>6 should be -1, but the test expects 1.

Do me a favor: have a look again and tell me whether you think I'm crazy now or was crazy in 2005. If you agree that I've grown less crazy as regard this code in the last 9 years, I will make a complete fix immediately.

karenetheridge commented 10 years ago

I'm going to go with my man Miller on this one: "If you realize that you aren't as wise today as you thought you were yesterday, you're wiser today." (Olin Miller) The rewritten overloads look correct.

The comments are backward (it's doing $x <=> 6 but the comment is 6 <=> it)

Er, no? (6 <=> $guess) should yield +1: 6 <=> 5.5 is +1. Where are you getting that the test description and expected value are backwards?

I do wonder though if you should just define the <=> overload and let the fallback logic take care of !=, ==, >, <, >=, <=

karenetheridge commented 10 years ago

I've worked through a few of the failing test files and rewritten the failing tests according to what I think they should return, and so far the new code is checking out:

diff --git a/t/less_than.t b/t/less_than.t
index 193cadb..41866db 100644
--- a/t/less_than.t
+++ b/t/less_than.t
@@ -45,8 +45,8 @@ ok(     5.0 >= $guess,    " ... 5.0 is more than or equal to it");
 ok(     5.5 >= $guess,    " ... 5.5 is more than or equal to it");
 ok(     5.6 >= $guess,    " ... 5.6 is more than or equal to it");

-is( (4 <=> $guess), -1,   " ... 4 <=> it is -1");
-is( (5 <=> $guess),  0,   " ... 5 <=> it is  0");
+is( (4 <=> $guess),  0,   " ... 4 <=> it is 0");
+is( (5 <=> $guess), +1,   " ... 5 <=> it is +1");
 is( (6 <=> $guess), +1,   " ... 6 <=> it is +1");

 # ... and now more of the same, BACKWARDS
@@ -83,8 +83,8 @@ ok(     $guess <= 5.0 ,   " ... it is more than or equal to 5.0");
 ok(     $guess <= 5.5 ,   " ... it is more than or equal to 5.5");
 ok(     $guess <= 5.6 ,   " ... it is more than or equal to 5.6");

-is( ( $guess <=> 4 ), +1, " ... 4 <=> it is -1");
-is( ( $guess <=> 5 ),  0, " ... 5 <=> it is  0");
+is( ( $guess <=> 4 ), 0, " ... 4 <=> it is 0");
+is( ( $guess <=> 5 ), -1, " ... 5 <=> it is -1");
 is( ( $guess <=> 6 ), -1, " ... 6 <=> it is +1");

 { # from_string
diff --git a/t/offset.t b/t/offset.t
index 59f8380..76c938b 100644
--- a/t/offset.t
+++ b/t/offset.t
@@ -35,7 +35,7 @@ ok(     5.6 > $guess,     " ... 5.6 is more than it");

 is( (4    <=> $guess), -1, " ... 4    <=> it is -1");
 is( (4.75 <=> $guess),  0, " ... 4.75 <=> it is 0");
-is( (5    <=> $guess), +1, " ... 5    <=> it is +1");
+is( (5    <=> $guess),  0, " ... 5    <=> it is 0");
 is( (6    <=> $guess), +1, " ... 6    <=> it is +1");

 # ... and now more of the same, BACKWARDS
@@ -62,7 +62,7 @@ ok(not( $guess > 5.6),    " ... it isn't more than 5.6");

 is( ($guess <=> 4   ), +1, " ... 4    <=> it is +1");
 is( ($guess <=> 4.75),  0, " ... 4.75 <=> it is  0");
-is( ($guess <=> 5   ), -1, " ... 5    <=> it is -1");
+is( ($guess <=> 5   ),  0, " ... 5    <=> it is  0");
 is( ($guess <=> 6   ), -1, " ... 6    <=> it is -1");

 { # from_string
karenetheridge commented 10 years ago

You're right about some of the tests being screwed up... t/infinite.t is downright loopy (and shows some additional errors in how infinity is treated).

And infinity isn't "any number" - it's infinity - greater than any real number. Mr. Signes, what were you smoking? :)

diff --git a/t/infinite.t b/t/infinite.t
index 0f6a114..2fc4f02 100644
--- a/t/infinite.t
+++ b/t/infinite.t
@@ -11,57 +11,57 @@ ok(defined $guess, "created our object");

 isa_ok($guess, "Number::Tolerant", " ... ");

-is("$guess", "any number", " ... stringifies properly");
+is("$guess", "infinity", " ... stringifies properly");

-ok(0.0 == $guess,         " ... 0.0 is equal to it");
-ok(4.4 == $guess,         " ... 4.4 is equal to it");
-ok(4.5 == $guess,         " ... 4.5 is equal to it");
-ok(5.0 == $guess,         " ... 5.0 is equal to it");
-ok(5.5 == $guess,         " ... 5.5 is equal to it");
-ok(5.6 == $guess,         " ... 5.6 is equal to it");
-ok(6.0 == $guess,         " ... 6.0 is equal to it");
+ok(not( 0.0 == $guess),   " ... 0.0 isn't equal to it");
+ok(not( 4.4 == $guess),   " ... 4.4 isn't equal to it");
+ok(not( 4.5 == $guess),   " ... 4.5 isn't equal to it");
+ok(not( 5.0 == $guess),   " ... 5.0 isn't equal to it");
+ok(not( 5.5 == $guess),   " ... 5.5 isn't equal to it");
+ok(not( 5.6 == $guess),   " ... 5.6 isn't equal to it");
+ok(not( 6.0 == $guess),   " ... 6.0 isn't equal to it");

-ok(not( 4.4 < $guess),    " ... 4.4 is less than it");
-ok(not( 4.5 < $guess),    " ... 4.5 isn't less than it");
-ok(not( 5.0 < $guess),    " ... 5.0 isn't less than it");
-ok(not( 5.5 < $guess),    " ... 5.5 isn't less than it");
-ok(not( 5.6 < $guess),    " ... 5.6 isn't less than it");
+ok(4.4 < $guess,          " ... 4.4 is less than it");
+ok(4.5 < $guess,          " ... 4.5 is less than it");
+ok(5.0 < $guess,          " ... 5.0 is less than it");
+ok(5.5 < $guess,          " ... 5.5 is less than it");
+ok(5.6 < $guess,          " ... 5.6 is less than it");

 ok(not( 4.4 > $guess),    " ... 4.4 isn't more than it");
 ok(not( 4.5 > $guess),    " ... 4.5 isn't more than it");
 ok(not( 5.0 > $guess),    " ... 5.0 isn't more than it");
 ok(not( 5.5 > $guess),    " ... 5.5 isn't more than it");
-ok(not( 5.6 > $guess),    " ... 5.6 is more than it");
+ok(not( 5.6 > $guess),    " ... 5.6 isn't more than it");

-is( (4 <=> $guess), 1,    " ... 4 <=> it is 0");
-is( (5 <=> $guess), 1,    " ... 5 <=> it is 0");
-is( (6 <=> $guess), 1,    " ... 6 <=> it is 0");
+is( (4 <=> $guess), -1,    " ... 4 <=> it is -1");
+is( (5 <=> $guess), -1,    " ... 5 <=> it is -1");
+is( (6 <=> $guess), -1,    " ... 6 <=> it is -1");

 # ... and now more of the same, BACKWARDS

-ok($guess == 0.0,         " ... it isn't equal to 0.0");
-ok($guess == 4.4,         " ... it isn't equal to 4.4");
-ok($guess == 4.5,         " ... it is equal to 4.5");
-ok($guess == 5.0,         " ... it is equal to 5.0");
-ok($guess == 5.5,         " ... it is equal to 5.5");
-ok($guess == 5.6,         " ... it isn't equal to 5.6");
-ok($guess == 6.0,         " ... it isn't equal to 6.0");
+ok(not( $guess == 0.0),   " ... it isn't equal to 0.0");
+ok(not( $guess == 4.4),   " ... it isn't equal to 4.4");
+ok(not( $guess == 4.5),   " ... it isn't equal to 4.5");
+ok(not( $guess == 5.0),   " ... it isn't equal to 5.0");
+ok(not( $guess == 5.5),   " ... it isn't equal to 5.5");
+ok(not( $guess == 5.6),   " ... it isn't equal to 5.6");
+ok(not( $guess == 6.0),   " ... it isn't equal to 6.0");

 ok(not( $guess < 4.4),    " ... it isn't less than 4.4");
 ok(not( $guess < 4.5),    " ... it isn't less than 4.5");
 ok(not( $guess < 5.0),    " ... it isn't less than 5.0");
 ok(not( $guess < 5.5),    " ... it isn't less than 5.5");
-ok(not( $guess < 5.6),    " ... it is less than 5.6");
+ok(not( $guess < 5.6),    " ... it isn't less than 5.6");

-ok(not( $guess > 4.4),    " ... it is more than 4.4");
-ok(not( $guess > 4.5),    " ... it isn't more than 4.5");
-ok(not( $guess > 5.0),    " ... it isn't more than 5.0");
-ok(not( $guess > 5.5),    " ... it isn't more than 5.5");
-ok(not( $guess > 5.6),    " ... it isn't more than 5.6");
+ok($guess > 4.4,    " ... it is more than 4.4");
+ok($guess > 4.5,    " ... it is more than 4.5");
+ok($guess > 5.0,    " ... it is more than 5.0");
+ok($guess > 5.5,    " ... it is more than 5.5");
+ok($guess > 5.6,    " ... it is more than 5.6");

-is( ($guess <=> 4), -1,   " ... 4 <=> it is 0");
-is( ($guess <=> 5), -1,   " ... 5 <=> it is 0");
-is( ($guess <=> 6), -1,   " ... 6 <=> it is 0");
+is( ($guess <=> 4),  1,   " ... it <=> 4 is 1");
+is( ($guess <=> 5),  1,   " ... it <=> 5 is 1");
+is( ($guess <=> 6),  1,   " ... it <=> 6 is 1");

 { # from_string
        my $tol = Number::Tolerant->from_string("any number");
karenetheridge commented 10 years ago

Heh, lib/Number/Tolerant/infinite.pm has: sub construct { shift; { value => 0 } } - that explains it!

karenetheridge commented 10 years ago
diff --git a/t/more_than.t b/t/more_than.t
index e855560..a04eee5 100644
--- a/t/more_than.t
+++ b/t/more_than.t
@@ -17,9 +17,9 @@ ok(0.0 != $guess,         " ... 0.0 isn't equal to it");
 ok(4.4 != $guess,         " ... 4.4 isn't equal to it");
 ok(4.5 != $guess,         " ... 4.5 isn't equal to it");
 ok(5.0 != $guess,         " ... 5.0 isn't equal to it");
-ok(5.5 == $guess,         " ... 5.5 is equal to it");
-ok(5.6 == $guess,         " ... 5.6 is equal to it");
-ok(6.0 == $guess,         " ... 6.0 is equal to it");
+ok(5.5 != $guess,         " ... 5.5 isn't equal to it");
+ok(5.6 != $guess,         " ... 5.6 isn't equal to it");
+ok(6.0 != $guess,         " ... 6.0 isn't equal to it");

 ok(     4.4 < $guess,     " ... 4.4 is less than it");
 ok(     4.5 < $guess,     " ... 4.5 is less than it");
@@ -29,9 +29,9 @@ ok(not( 5.6 < $guess),    " ... 5.6 isn't less than it");

 ok(     4.4 <= $guess,    " ... 4.4 is less than or equal to it");
 ok(     4.5 <= $guess,    " ... 4.5 is less than or equal to it");
-ok(     5.0 <= $guess,    " ... 5.0 is less than or equal to it");
-ok(     5.5 <= $guess,    " ... 5.5 is less than or equal to it");
-ok(     5.6 <= $guess,    " ... 5.6 is less than or equal to it");
+ok(not( 5.0 <= $guess),   " ... 5.0 is less than or equal to it");
+ok(not( 5.5 <= $guess),   " ... 5.5 isn't less than or equal to it");
+ok(not( 5.6 <= $guess),   " ... 5.6 isn't less than or equal to it");

 ok(not( 4.4 > $guess),    " ... 4.4 isn't more than it");
 ok(not( 4.5 > $guess),    " ... 4.5 isn't more than it");
@@ -46,8 +46,8 @@ ok(     5.5 >= $guess,    " ... 5.5 is more than or equal to it");
 ok(     5.6 >= $guess,    " ... 5.6 is more than or equal to it");

 is( (4 <=> $guess), -1,   " ... 4 <=> it is -1");
-is( (5 <=> $guess),  0,   " ... 5 <=> it is  0");
-is( (6 <=> $guess), +1,   " ... 6 <=> it is +1");
+is( (5 <=> $guess), -1,   " ... 5 <=> it is  0");
+is( (6 <=> $guess),  0,   " ... 6 <=> it is  01");

 # ... and now more of the same, BACKWARDS

@@ -65,27 +65,27 @@ ok(not( $guess < 5.0),    " ... it isn't less than 5.0");
 ok(not( $guess < 5.5),    " ... it isn't less than 5.5");
 ok(not( $guess < 5.6),    " ... it isn't less than 5.6");

-ok(not( $guess <= 4.4),   " ... it isn't less than or equal 4.4");
-ok(not( $guess <= 4.5),   " ... it isn't less than or equal 4.5");
-ok(not( $guess <= 5.0),   " ... it is less than or equal 5.0");
-ok(     $guess <= 5.5,    " ... it is less than or equal 5.5");
-ok(     $guess <= 5.6,    " ... it is less than or equal 5.6");
+ok(not( $guess <= 4.4),   " ... it isn't less than or equal to 4.4");
+ok(not( $guess <= 4.5),   " ... it isn't less than or equal to 4.5");
+ok(not( $guess <= 5.0),   " ... it is less than or equal to 5.0");
+ok(     $guess <= 5.5,    " ... it is less than or equal to 5.5");
+ok(     $guess <= 5.6,    " ... it is less than or equal to 5.6");

 ok(     $guess > 4.4,     " ... it is more than 4.4");
 ok(     $guess > 4.5,     " ... it is more than 4.5");
 ok(     $guess > 5.0,     " ... it is more than 5.0");
-ok(not( $guess > 5.5),    " ... it isn't more than 5.5");
-ok(not( $guess > 5.6),    " ... it isn't more than 5.6");
+ok(not( $guess > 5.5),    " ... it is more than 5.5");
+ok(not( $guess > 5.6),    " ... it is more than 5.6");

-ok(     $guess >= 4.4,    " ... it is more than or equal 4.4");
-ok(     $guess >= 4.5,    " ... it is more than or equal 4.5");
-ok(     $guess >= 5.0,    " ... it is more than or equal 5.0");
-ok(     $guess >= 5.5,    " ... it is more than or equal 5.5");
-ok(     $guess >= 5.6,    " ... it is more than or equal 5.6");
+ok(     $guess >= 4.4,    " ... it is more than or equal to 4.4");
+ok(     $guess >= 4.5,    " ... it is more than or equal to 4.5");
+ok(     $guess >= 5.0,    " ... it is more than or equal to 5.0");
+ok(not( $guess >= 5.5),   " ... it isn't more than or equal to 5.5");
+ok(not( $guess >= 5.6),   " ... it isn't more than or equal to 5.6");

-is( ($guess <=> 4), +1,   " ... 4 <=> it is -1");
-is( ($guess <=> 5),  0,   " ... 5 <=> it is  0");
-is( ($guess <=> 6), -1,   " ... 6 <=> it is +1");
+is( ($guess <=> 4), +1,   " ... it <=> 4 is +1");
+is( ($guess <=> 5), +1,   " ... it <=> 5 is +1");
+is( ($guess <=> 6), 0,    " ... it <=> 6 is 0");

 { # from_string
   { # prosaic
karenetheridge commented 10 years ago

I've pushed what I have so far to https://github.com/karenetheridge/Number-Tolerant/tree/topic/oh_dear although I haven't read any of the documentation, so my interpretation of some of the results may not match. I shall reconsider in the light of this :)

rjbs commented 10 years ago

Er, no? (6 <=> $guess) should yield +1: 6 <=> 5.5 is +1. Where are you getting that the test description and expected value are backwards?

I had stupidly quoted an example that DIDN'T have the problem! HA!

# from t/offset.t
is( ($guess <=> 4   ), +1, " ... 4    <=> it is +1");                             
is( ($guess <=> 4.75),  0, " ... 4.75 <=> it is  0");                             
is( ($guess <=> 5   ), -1, " ... 5    <=> it is -1");                             
is( ($guess <=> 6   ), -1, " ... 6    <=> it is -1");                             
rjbs commented 9 years ago

I'm closing this ticket, because it claims too many things, some of them clearly incorrect. For example:

And infinity isn't "any number" - it's infinity - greater than any real number. Mr. Signes, what were you smoking? :)

An infinite tolerance is not infinity. It is a tolerance for any value. So, yes, it is any number.

How do you compare it with > and <? Nothing is greater or less than it. We only implement these because we must. Otherwise, they'd be left undefined. Perl 5 doesn't have "can compare for numeric equality but not sort" role.

In general, tolerances sort by their chief value, but not all tolerances have one. For example, (5 -10 +5) is targeting 5, but will allow -5 or 10. The midpoint is 2.5, but the value for sorting is 5. Basically, the sorting methods are there only to make things slightly less crazy. They do not have, nor I think can they have, some canonically correct and extremely obvious behavior.

If there are specific failures, please point them out. There may be some above, but if so, I think I've lost sight of them.

karenetheridge commented 9 years ago

In general, tolerances sort by their chief value

This appears to contradict your earlier statements about <=> -- https://github.com/rjbs/Number-Tolerant/issues/2#issuecomment-48137393. Which is correct?

I'm trying out a new branch that makes that change to <=>, and on the whole, the results look a lot more sane.

An infinite tolerance is not infinity. It is a tolerance for any value. So, yes, it is any number.

This deserves being spelled out explicitly in the documentation, as that's not how I interpreted the use of infinity in a tolerance.

rjbs commented 9 years ago

This appears to contradict your earlier statements about <=> -- #2 (comment). Which is correct?

My later statement is correct.

You say that you have changes that make things "look more sane," but this implies that there is some "canonically correct and extremely obvious behavior" for sorting, which I said I didn't think existed. Or at least it implies that some behaviors are way better than others… but I'm not sure what you're seeing that's obviously better.

I agree that <=> should always return 0 for an infinite tolerance, though, as your test suggested.

My big question here, though is: what is the use case for sorting tolerances?

karenetheridge commented 9 years ago

Sorting doesn't have anything to do with this. That's just one use of <=>. The change, rather, aims to make the behaviour of the spaceship operator consistent with the behaviour of <, == and >, where previously it was not.

That is: if $a < $b is true, then $a <=> $b should ALWAYS return -1, surely?

rjbs commented 8 years ago

fixed by #5