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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:50 | admin | set | github: 50680 |
2009-07-18 15:22:00 | mark.dickinson | set | status: open -> closed resolution: fixed messages:
+ msg90683
stage: patch review -> resolved |
2009-07-18 14:47:18 | mark.dickinson | set | messages:
+ msg90680 |
2009-07-16 22:55:26 | casevh | set | messages:
+ msg90595 |
2009-07-16 22:27:58 | jyasskin | set | messages:
+ msg90593 |
2009-07-16 22:10:02 | mark.dickinson | set | messages:
+ msg90591 |
2009-07-16 20:36:43 | jyasskin | set | messages:
+ msg90582 |
2009-07-16 18:34:30 | mark.dickinson | set | files:
+ issue6431.patch
messages:
+ msg90576 stage: test needed -> patch review |
2009-07-16 05:22:34 | casevh | set | files:
+ patch_test_fractions.py
messages:
+ msg90554 |
2009-07-08 12:37:43 | mark.dickinson | set | messages:
+ msg90265 |
2009-07-08 07:36:48 | mark.dickinson | set | priority: normal assignee: mark.dickinson messages:
+ msg90254
stage: needs patch -> test needed |
2009-07-08 03:08:24 | casevh | set | files:
+ fractions_patch.diff keywords:
+ patch messages:
+ msg90250
|
2009-07-07 15:45:02 | casevh | set | messages:
+ msg90241 |
2009-07-07 08:32:42 | mark.dickinson | set | stage: needs patch versions:
+ Python 2.6, Python 2.7, Python 3.2 |
2009-07-07 08:26:07 | mark.dickinson | set | nosy:
+ mark.dickinson, jyasskin messages:
+ msg90216
|
2009-07-07 05:41:34 | casevh | create | |