classification
Title: Fraction fails equality test with a user-defined type
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: casevh, jyasskin, mark.dickinson
Priority: normal Keywords: patch

Created on 2009-07-07 05:41 by casevh, last changed 2009-07-18 15:22 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
fractions_patch.diff casevh, 2009-07-08 03:08
patch_test_fractions.py casevh, 2009-07-16 05:22 Test Fraction comparison with user-defined types.
issue6431.patch mark.dickinson, 2009-07-16 18:34
Messages (14)
msg90211 - (view) Author: Case Van Horsen (casevh) Date: 2009-07-07 05:41
I've ported the GMPY module to Python 3 and found a problem comparing
Fraction to gmpy.mpq. mpq is the rational type in gmpy and knows how to
convert a Fraction into an mpq. All operations appear to work properly
except "Fraction == mpq".

"mpq == Fraction" does work correctly. gmpy's rich comparison routine
recognizes the other argument as Fraction and converts to an mpq value
properly. However, when "Fraction == mpq" is done, the Fraction argument
is converted to a float before gmpy's rich comparison is called.

The __eq__ routine in fractions.py is:

    def __eq__(a, b):
        """a == b"""
        if isinstance(b, numbers.Rational):
            return (a._numerator == b.numerator and
                    a._denominator == b.denominator)
        if isinstance(b, numbers.Complex) and b.imag == 0:
            b = b.real
        if isinstance(b, float):
            return a == a.from_float(b)
        else:
            # XXX: If b.__eq__ is implemented like this method, it may
            # give the wrong answer after float(a) changes a's
            # value. Better ways of doing this are welcome.
            return float(a) == b

Shouldn't __eq__ return NotImplemented if it doesn't know how to handle
the other argument? I changed "return float(a) == b" to "return
NotImplemented" and GMPY and Python's test suite passed all tests.

I used the same logic for comparisons between gmpy.mpf and Decimal and
they all work correctly. Decimal does return NotImplemented when it
can't convert the other argument.

(GMPY 1.10 alpha2 fails due to this issue.)
msg90216 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-07 08:26
I agree this should be fixed.  The conversion to float in the else 
clause seems wrong to me: it can lose precision, making two unequal 
values compare equal.  I also agree that we should be getting 
NotImplemented here.

Do you have a patch available?

As an aside, I'm interested that you're implementing comparisons between 
mpf and Decimal;  how does that work?  Do you also implement binary 
arithmetic operations between mpf and Decimal?  What's the result type?
msg90241 - (view) Author: Case Van Horsen (casevh) Date: 2009-07-07 15:45
On Tue, Jul 7, 2009 at 1:26 AM, Mark Dickinson<report@bugs.python.org> wrote:
>
> Mark Dickinson <dickinsm@gmail.com> added the comment:
>
> I agree this should be fixed.  The conversion to float in the else
> clause seems wrong to me: it can lose precision, making two unequal
> values compare equal.  I also agree that we should be getting
> NotImplemented here.
>
> Do you have a patch available?

I'll upload a patch this evening.

>
> As an aside, I'm interested that you're implementing comparisons between
> mpf and Decimal;  how does that work?  Do you also implement binary
> arithmetic operations between mpf and Decimal?  What's the result type?

When I do binary operations (including comparison), I check the type
of both operands. If both operands are Integer (mpz, int, long), I
convert both operands to an mpz and then perform the operation.
Otherwise, if both operands are Rational (mpz, int, long, mpq,
Fraction), I convert both operands to an mpq. Finally, if both
operands appear to be a Number (mpz, int, long, mpq, Fraction, mpf,
Decimal), I convert both operands to an mpf. In the latest release of
GMPY, I always return a GMPY type (unless you are converting to float
or int/long).

To do the conversions, I just do mpq(str(Fraction)) or
mpf(str(Decimal)). I realize that the conversions aren't perfect but
are probably what a user expects. The fact that GMP uses radix 2^32 or
2^64 floating point with (randomly) either [prec+1] or [prec+2] digits
makes predictable floating point challenging. ;-) (prec =
floor(precision_in_bits/limb_size).) In GMPY 1.04, I forced all mpf
results to have [prec+1] digits but it's still messy. Time to start
working on MPFR support.
>
> ----------
> nosy: +jyasskin, marketdickinson
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue6431>
> _______________________________________
>
msg90250 - (view) Author: Case Van Horsen (casevh) Date: 2009-07-08 03:08
Change Fraction __eq__ method to give the other operand a chance to
perform the comparison if Fraction doesn't understand the other operand.
msg90254 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-08 07:36
Thanks.  Could you provide some tests?  (If you don't have time then 
that's fine: I'll probably get to this eventually, but it might take a 
while...)
msg90265 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-08 12:37
A good solution should ensure that all 6 comparison operators behave in 
the same way for the same input types:  that is, if x == y returns 
NotImplemented for some particular Python objects x and y, then x < y, x 
<= y, etc. should also return NotImplemented, and vice versa.
msg90554 - (view) Author: Case Van Horsen (casevh) Date: 2009-07-16 05:22
I've attached a patch that creates DummyRational and then tests
comparisons between Fraction and DummyRational. The __eq__ method also
verifies that the type of the other argument is fractions.Fraction.
msg90576 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-16 18:34
Thanks again, casevh!  The patch looks good.  I've added to it a bit,
though---see issue6431.patch.  In detail:

- don't use subtraction with unknown types for <, <=, >, >=;  this is
  dangerous, since the unknown type may well do a lossy conversion, and
  comparisons should really be exact where possible;  as with __eq__,
  it seems better to return NotImplemented and give the other type a
  chance.

- handle infs and nans correctly in comparisons with floats

- a few more tests.

casevh, please could you have a look at the attached patch and let me
know whether it still works with your gmpy port?

Jeffrey, any comments on these changes?
msg90582 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2009-07-16 20:36
The fallback behavior in Fraction was meant to demonstrate the suggested 
fallback behavior for user-defined types. In particular, the idea was 
that all Reals would (by default) be comparable to each other, even if 
they didn't know about each other. Unfortunately, the comparison protocol 
provides no way to know if a particular call is the first call or the 
fallback, so __eq__(Fraction, b) has to return the same thing as 
__eq__(b, Fraction). If b doesn't know about Fraction, and Fraction wants 
to make a best attempt at comparing to it, then __eq__(Fraction, b) can't 
return NotImplemented.
msg90591 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-16 22:10
[Jeffrey]
> In particular, the idea was that all Reals would (by default) be
> comparable to each other, even if they didn't know about each other.

Understood, but I don't think this is an attainable goal.  I don't see 
any reasonable way to make it happen without doing significant guessing, 
or expanding the numbers ABC in some way.

At the moment, __eq__(a, b) falls back to 'float(a) == b' when b is not 
a float or an instance of Rational.  This seems problematic to me for a 
couple of reasons:

1. The conversion to float loses information.  As a result, we lose (a) 
transitivity of equality, (b) well-behaved hashing (x == y no longer 
implies hash(x) == hash(y)), and (c) consistency between == and the 
other comparison operators.

2. This fallback shuts out the other class even in cases where the other 
class *does* know how to handle the comparison.  So there's no way for 
another class to 'play nice' with the Fraction type and implement exact 
comparisons even if it wants to.

Here's an example of 1, on Python 2.6.  (bigfloat is a home-built 
wrapper for the MPFR library.)

newton:~ dickinsm$ python2.6
Python 2.6.2 (r262:71600, Jun 17 2009, 09:08:27) 
[GCC 4.0.1 (Apple Inc. build 5490)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from bigfloat import BigFloat
>>> from fractions import Fraction
>>> x = Fraction(2**60-1)
>>> y = Fraction(2**60+1)
>>> z = BigFloat(2**60)
>>> x == z
True
>>> y == z
True
>>> x == y
False
>>> hash(x) == hash(z)
False

I just don't see any reasonable way to make comparisons 'automatically' 
work:  one of the classes has to know how to handle both types, or else 
there's just going to be a lot of guesswork involved.  So it seems 
better to simply return NotImplemented in these cases.
msg90593 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2009-07-16 22:27
If you think it's better, I'm happy to make the other tradeoff.
msg90595 - (view) Author: Case Van Horsen (casevh) Date: 2009-07-16 22:55
On Thu, Jul 16, 2009 at 11:34 AM, Mark Dickinson<report@bugs.python.org> wrote:
>
> Mark Dickinson <dickinsm@gmail.com> added the comment:
>
> Thanks again, casevh!  The patch looks good.  I've added to it a bit,
> though---see issue6431.patch.  In detail:
>
> - don't use subtraction with unknown types for <, <=, >, >=;  this is
>  dangerous, since the unknown type may well do a lossy conversion, and
>  comparisons should really be exact where possible;  as with __eq__,
>  it seems better to return NotImplemented and give the other type a
>  chance.
>
> - handle infs and nans correctly in comparisons with floats
>
> - a few more tests.
>
> casevh, please could you have a look at the attached patch and let me
> know whether it still works with your gmpy port?

I've tested gmpy with attached patch and all tests pass successfully.
Thanks!

>
> Jeffrey, any comments on these changes?
>
> ----------
> stage: test needed -> patch review
> Added file: http://bugs.python.org/file14508/issue6431.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue6431>
> _______________________________________
>
msg90680 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-18 14:47
Applied in r74078 (py3k), r74079 (release31-maint).  I'll backport to 2.x.
msg90683 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-07-18 15:22
Backported to trunk in r74080.  I don't think it's worth fixing this in 
2.6:  it seems unlikely that the changed comparison behaviour would cause 
breakage, but I don't want to take the chance.

Thanks Case for the report and patches!
History
Date User Action Args
2009-07-18 15:22:00mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg90683

stage: patch review -> resolved
2009-07-18 14:47:18mark.dickinsonsetmessages: + msg90680
2009-07-16 22:55:26casevhsetmessages: + msg90595
2009-07-16 22:27:58jyasskinsetmessages: + msg90593
2009-07-16 22:10:02mark.dickinsonsetmessages: + msg90591
2009-07-16 20:36:43jyasskinsetmessages: + msg90582
2009-07-16 18:34:30mark.dickinsonsetfiles: + issue6431.patch

messages: + msg90576
stage: test needed -> patch review
2009-07-16 05:22:34casevhsetfiles: + patch_test_fractions.py

messages: + msg90554
2009-07-08 12:37:43mark.dickinsonsetmessages: + msg90265
2009-07-08 07:36:48mark.dickinsonsetpriority: normal
assignee: mark.dickinson
messages: + msg90254

stage: needs patch -> test needed
2009-07-08 03:08:24casevhsetfiles: + fractions_patch.diff
keywords: + patch
messages: + msg90250
2009-07-07 15:45:02casevhsetmessages: + msg90241
2009-07-07 08:32:42mark.dickinsonsetstage: needs patch
versions: + Python 2.6, Python 2.7, Python 3.2
2009-07-07 08:26:07mark.dickinsonsetnosy: + mark.dickinson, jyasskin
messages: + msg90216
2009-07-07 05:41:34casevhcreate