Issue6431

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) * | 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! |

History | |||
---|---|---|---|

Date | User | Action | Args |

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 |