Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

float compared to decimal is silently incorrect. #46783

Closed
jdunck mannequin opened this issue Apr 1, 2008 · 35 comments
Closed

float compared to decimal is silently incorrect. #46783

jdunck mannequin opened this issue Apr 1, 2008 · 35 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jdunck
Copy link
Mannequin

jdunck mannequin commented Apr 1, 2008

BPO 2531
Nosy @rhettinger, @facundobatista, @mdickinson, @skrah
Files
  • decimal.patch: always raise exception if 'other' can't be converted
  • float_decimal_comparisons.patch
  • issue2531.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/mdickinson'
    closed_at = <Date 2010-04-03.11:45:02.858>
    created_at = <Date 2008-04-01.22:11:36.754>
    labels = ['type-bug', 'library']
    title = 'float compared to decimal is silently incorrect.'
    updated_at = <Date 2010-04-03.11:45:02.857>
    user = 'https://bugs.python.org/jdunck'

    bugs.python.org fields:

    activity = <Date 2010-04-03.11:45:02.857>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2010-04-03.11:45:02.858>
    closer = 'mark.dickinson'
    components = ['Library (Lib)']
    creation = <Date 2008-04-01.22:11:36.754>
    creator = 'jdunck'
    dependencies = []
    files = ['9922', '13389', '16544']
    hgrepos = []
    issue_num = 2531
    keywords = ['patch']
    message_count = 35.0
    messages = ['64827', '64855', '83691', '83694', '83816', '83818', '83935', '83939', '83940', '83941', '83942', '83944', '83948', '83950', '83951', '83952', '84433', '85099', '86053', '86058', '86059', '86061', '86123', '86438', '86439', '95263', '97891', '97935', '98215', '98217', '98218', '101053', '101231', '102147', '102242']
    nosy_count = 9.0
    nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'jdunck', 'lorg', 'skrah', 'adamtj', 'bertchughes', 'bas']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue2531'
    versions = ['Python 3.2']

    @jdunck
    Copy link
    Mannequin Author

    jdunck mannequin commented Apr 1, 2008

    Within python 2.5.2:

    >>> from decimal import Decimal
    >>> x = 3.0
    >>> y = Decimal('0.25')
    >>> x > y
    False (expected error, as in 2.4, or True)

    @jdunck jdunck mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 1, 2008
    @facundobatista
    Copy link
    Member

    Which version of 2.4 are you talking about?

    This line in the tests...

      self.assertNotEqual(da, 32.7)

    is there since almost 4 years ago (release 36325, and 2.4 is tagged in
    release 37906).

    Anyway, this behaviour is ok, as is the following:

    >>> 2 < "1.9"
    True

    @lorg
    Copy link
    Mannequin

    lorg mannequin commented Mar 17, 2009

    This behavior also exists in Python 2.6. However, in Python 3 an
    exception is raised instead, as these kinds of comparisons are
    considered incorrect.
    Because of that, I'd like Python 3's behavior of raising exceptions on
    float-decimal comparisons to be backported to Python 2.6.

    As an aside, regardless of Python 3's behavior, there is a big
    difference between 2<"1.9" and 1.6 < decimal("2.0"). It seems reasonable
    to expect decimal comparisons to be made according to numerical value.
    When this doesn't happen, it is better to fail loudly rather than silently.

    @rhettinger
    Copy link
    Contributor

    Imri, I don't think the 3.0 model for cross-type comparisons can be
    backported without breaking code, so it probably isn't going to happen.
    The whole purpose of the 3.x series was to allow improvements that
    weren't backwards compatible.

    Mark, this raises a question for us. Now that we have
    decimal.from_float(), we do have a means of making exact comparisons
    between decimals and floats. While I think cross-type interaction is
    generally a bad idea, the 2.x way of doing things already gives an
    answer (though somewhat useless). What are your thoughts on making
    Decimal('0.80') < float('0.75') do the right thing in the 2.x series so
    that the answer that is given won't be flat-out wrong.

    @mdickinson
    Copy link
    Member

    Making float <-> Decimal comparisons return the 'right' answer in 2.x does
    look attractive at first sight, but the more I think about it the more it
    seems a bad idea. Having the comparisons work in 2.x but not in 3.x seems
    especially nasty, and allowing mixed-type comparisons but not mixed-type
    arithmetic operations also seems somehow unclean. So -1 from me. (And
    no, I don't want to add full float <-> Decimal interaction: the Decimal
    module is quite complicated enough as it is.)

    Are there many cases where float <-> Decimal comparisons are useful? The
    only uses I can think of would also involve a need for mixed-type
    arithmetic. In the few cases where it's really needed I don't think it's
    a problem to explicitly convert one or the other type.

    The current bogus comparison results also suck, but they're just one
    aspect of a known Python 2.x gotcha.

    Would it be possible to raise a warning for Decimal <-> float comparisons?
    Does -3 already do this?

    @jdunck
    Copy link
    Mannequin Author

    jdunck mannequin commented Mar 19, 2009

    I hear you on the 2.x to 3.x transition-- I'm not really asking for
    mixed-mode arithmetic. I'd be perfectly happy if float > decimal raised
    TypeError, as float + decimal does.

    My complaint is that it is silently wrong.

    @mdickinson
    Copy link
    Member

    My complaint is that it is silently wrong.

    I appreciate that, but I don't see any good solution. Yes, it would be
    nice if float <-> Decimal comparisons raised TypeError in 2.x. But Python
    2.x has an '(almost) everything is comparable to everything else'
    comparison model, so that for example a list of arbitrary Python objects
    can almost always be sorted. So raising a TypeError for these comparisons
    has the potential to break already existing code. Of course, it *might*
    not cause any breakage at all, but it's difficult to see how one could
    ever be sure of that.

    I think the best we can do would be to add a warning for float <-> Decimal
    comparisons. Facundo, Raymond: does this seem reasonable?

    @rhettinger
    Copy link
    Contributor

    I think "the best we can do" is return valid comparison results between
    floats and decimals in 2.x. It doesn't make anything worse and it does
    make something better. Unlike other cross-type comparisons,
    number-to-number is not an unreasonable thing to do. And, we not
    obliged to carry that over to 3.x where cross-type comparisons have to
    be specifically enabled. I believe this is the best solution.

    @rhettinger
    Copy link
    Contributor

    The next to last sentence should have read "and, we are not
    obliged to carry that over to 3.x where cross-type ordering comparisons
    are not the norm unless a type has specifically enabled them."

    The gist of the idea is that in 2.x, we do have cross-type ordering
    comparisons so we should make them as useful and non-misleading as
    possible. But, in 3.x there is no default cross-type ordering
    comparisons so we're not obliged to return any answer at all.

    @mdickinson
    Copy link
    Member

    What about Decimal <-> Fraction comparisons?

    @rhettinger
    Copy link
    Contributor

    It's not a priority for me though it's not an unreasonable thing to do.
    A basic 5th grade exercise is ordering fractions, sometimes with their
    decimal equivalents: Fraction(1,3) < Decimal('0.4') < Fraction(1,2).
    I don't care if that gets done.

    If you do decide to do fractions too, the responsibility should be with
    the fractions class (since decimals convert to fractions exactly but not
    vice-versa).

    @mdickinson
    Copy link
    Member

    Here's a patch.

    I'm still not 100% convinced this is a good idea. Part of my objection is
    that it seems likely that these comparisons are fairly useless, in that a
    mixed-type comparison is probably going to be followed by a mixed-type
    arithmetic operation at some point (unless people are misspelling "x < 0"
    as "x < 0.0"). So all that's really gained is a noisy failure instead of
    a silent one. Still, I suppose that's something.

    @mdickinson mdickinson reopened this Mar 21, 2009
    @mdickinson
    Copy link
    Member

    On the other hand, if it's true that mixed-type comparisons are generally
    followed by mixed-type arithmetic, then these comparisons just become a
    roundabout way to raise TypeError, which is what everybody wanted in the
    first place. :-)

    The patch still needs docs.

    @rhettinger rhettinger assigned rhettinger and unassigned mdickinson Mar 21, 2009
    @mdickinson
    Copy link
    Member

    Urk. That patch produces horrible results when comparing Fractions and
    Decimals:

    Python 2.7a0 (unknown, Mar 21 2009, 17:59:48) 
    [GCC 4.0.1 (Apple Inc. build 5490)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from fractions import Fraction
    >>> from decimal import Decimal
    >>> Decimal('2.5') == Fraction(5, 2)
    True
    >>> Decimal('1.1') == Fraction(11, 10)
    False

    Either both results should be True (if comparisons between Fractions and
    Decimals work numerically), or both should be False (refuse to
    compare Fraction and Decimal).

    It looks like what happens is that the Fraction comparison converts the
    second argument to float before comparing. I'm tempted to call this a
    bug in Fraction.__eq__.

    @mdickinson
    Copy link
    Member

    One more consideration: if Decimal('2.5') == 2.5 is True, should we also
    be fixing the Decimal hash method so that hash(Decimal('2.5')) ==
    hash(2.5)?

    @rhettinger
    Copy link
    Contributor

    I'll look at this more later today.

    @mdickinson
    Copy link
    Member

    Removing easy keyword, since I don't think it applies here.

    The problem here is hashing: either we break the rule that
    if two objects compare equal then they hash equal, or we
    fix hash so that e.g., hash(Decimal('2.5')) == hash(2.5).

    For the latter, the least invasive way to do it would be
    to fix only the Decimal __hash__ method. For that, we
    really need a Decimal -> float conversion, so that we
    can do something like (for a Decimal x):

    if x == Decimal.from_float(x.to_float()):
        return hash(x.to_float())
    [rest of hash method here]

    The builtin float() (which converts a Decimal to a string
    and then uses the standard C library's string -> float
    conversion) probably isn't good enough for this, since
    there are no requirements that it should be (even close to)
    correctly rounded.

    The bottom line: getting a correctly-rounded
    Decimal -> float method, without knowing what
    the float format is, is going to be hard.
    If we assume IEEE 754 then it's much easier.

    @mdickinson mdickinson removed the easy label Mar 29, 2009
    @rhettinger
    Copy link
    Contributor

    Mark, I looked at your patch again and think we should support relaxed
    conversions for ordering comparisons but not equality/inequality. This
    will improve on the current situation where we get flat-out misleading
    results for <, <=, >, and >=. It keeps the status quo for
    equality/inequality and thereby avoids the problems with __hash__.

    The only thing that I don't like about it is that you can't do the usual
    reasoning where "not a<b and not a>b" implies "a==b". Still, it is an
    improvement over "a<b" giving a completely useless and misleading result.

    @rhettinger rhettinger assigned mdickinson and unassigned rhettinger Apr 1, 2009
    @rhettinger
    Copy link
    Contributor

    Mark, any thoughts? I would like to apply this patch for ordering
    comparisons other than __eq__ and __ne__.

    @mdickinson
    Copy link
    Member

    On Fri, Apr 17, 2009 at 3:45 AM, Raymond Hettinger
    <report@bugs.python.org> wrote:

    Mark, any thoughts?  I would like to apply this patch for ordering
    comparisons other than __eq__ and __ne__.

    Hi Raymond,

    Sorry for not responding to this sooner. I'll post something to
    the issue tracker. In brief, this seems an improvement over
    the earlier proposal, just because there are no problems with
    hashes or container confusion. I'm still -0 on it, though---it
    just seems like too much magic: confusion will happen
    much more rarely, but it'll be that much more difficult to
    explain when it does occur.

    IOW I wouldn't do this if it were just up to me, but not going
    to object if there's support from you and others.

    Mark

    @rhettinger
    Copy link
    Contributor

    I'm not seeing the downside. This gives better answers than it does now
    but doesn't commit us to anything else.

    @mdickinson
    Copy link
    Member

    The downside is the potential confusion arising from
    using one method (comparison of actual numerical value)
    for <, <=, >, >=, and a different method (decimals
    and floats are never equal) for == and !=.

    @mdickinson
    Copy link
    Member

    Unassigning myself.

    Does anyone beside Raymond and me have strong opinions about how/whether
    this problem should be fixed?

    @mdickinson mdickinson removed their assignment Apr 18, 2009
    @rhettinger
    Copy link
    Contributor

    Closing since no one seems interested.

    @jdunck
    Copy link
    Mannequin Author

    jdunck mannequin commented Apr 24, 2009

    I'm interested. I just had already said my peace and didn't know my
    prior interest wasn't being counted. The patch uploaded by dmmartins
    seemed good to me. I'm probably biased, since this bug affected me.

    @mdickinson
    Copy link
    Member

    Just closed bpo-7323 as a duplicate of this one.

    I think this issue is worth reopening: with the backport of the py3k
    correctly rounded string <-> float conversions, there might now be a
    reasonable way to rewrite Decimal.__hash__ so that it's consistent with
    float.__hash__. Then we can make Decimal-to-float comparisons behave
    correctly and clear up this mess.

    I'd still be uncomfortable with allowing Decimal-to-float comparisons in
    2.x but not in 3.x. Maybe they could be permitted in 3.x too?

    @mdickinson mdickinson reopened this Nov 14, 2009
    @bertchughes
    Copy link
    Mannequin

    bertchughes mannequin commented Jan 16, 2010

    Expressions like "Decimal('100.0') < .01" being silently wrong (True!) is *very* dangerous behavior, it should raise exception like any other Decimal-float operation, and hopefully will be back-ported to 2.7.

    Eg: 3rd party module kinterbasdb, which provides access to Firebird database, returns floats from firebird large-int types (eg NUMERIC 18,2) in versions of kinrebasdb 3.2 or less, but in versions 3.3+ kinterbasdb retrieves large-int as type Decimal. This means if python/kinterbasdb users upgrade kinterbasdb they must be aware of this python bug, because all existing code must be inspected for "(retrieved Decimal value) compare (float)" statements, which before upgrade were Ok (retrieved float value) compare (float)) statements.

    I'm new to this tracker, I hope this simply is added as an additional comment & squawk of dismay to the "float compared to decimal is silently incorrec" issue.

    @mdickinson
    Copy link
    Member

    I'll try to find time to look at this again before 2.7 beta.

    @mdickinson mdickinson self-assigned this Jan 17, 2010
    @mdickinson
    Copy link
    Member

    So here's the plan for trunk. The big question is: what should be done for py3k?

    For trunk:

    • all comparisons (==, !=, <=, >=, <, >) between floats and Decimals
      return the correct numerical result, as though the float were
      converted to a Decimal with no loss of precision. Like-signed float
      and Decimal infinities compare equal. float and Decimal nans compare
      unequal to everything else, as always.

    • check whether anything special needs to be done to make sure that
      cmp also returns sensible results.

    • fix Decimal.__hash__ so that when x == y for a float x and Decimal
      instance y, hash(x) == hash(y)

    • the hash fix above is going to slow down hash computations, so
      consider caching hash values for Decimal instances (as a _hash
      attribute on the Decimal object itself). My gut feeling is that
      this probably isn't worth it, given that Decimal objects probably
      don't get hashed very often in normal use, but I'll do some timings
      to find out what the performance impact of the new hash is.

    For py3k, the obvious options are:

    (A) adopt the above changes, or

    (B) keep Decimal and float non-comparable (as currently). In this case, a Decimal-to-float comparison in trunk should probably raise a DeprecationWarning. (Note that DeprecationWarnings are now silent by default, so such a warning wouldn't impact end-users much.)

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 24, 2010

    I'm new to this thread, so I hope I didn't miss anything that has been
    said already. I don't fully understand why TypeError cannot be raised
    in 2.x. The 2.6 documentation for tp_richcompare says:

    "If you want to implement a type for which only a limited set of comparisons makes sense (e.g. == and !=, but not < and friends), directly raise TypeError in the rich comparison function."

    I just tried that in the 2.7 version of cdecimal, and it works well:

    >>> from cdecimal import *
    >>> Decimal(9) < 2.5
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: conversion from float to Decimal is not supported

    @mdickinson
    Copy link
    Member

    Stefan: the problem is backwards compatibility. In 2.6 it's possible to sort a heterogeneous list that contains both Decimal instances and floats. The resulting order may not be particularly meaningful, but for some applications that doesn't matter.

    If we make a Decimal-to-float comparison raise TypeError for 2.7 then sort will raise a TypeError where it used to work, so it's a potential code-breaking change. We could deprecate: raise a warning in 2.7 and make it a TypeError in 2.8, but since 2.8 currently seems unlikely to happen that would be a bit pointless.

    @mdickinson
    Copy link
    Member

    Here's a patch:

    • comparisons between a Decimal and float produce a result based
      on the numeric values of the Decimal and float

    • change Decimal.__hash__ so that floats and Decimals with (exactly)
      equal value have the same hash. This preserves the rule that if
      two objects compare equal then they hash equal.

    • add tests, and update documentation.

    Still open: should this change be forward ported to py3k? If not, then these comparisons should produce a DeprecationWarning.

    @mdickinson
    Copy link
    Member

    For anyone interested, there's an ongoing python-dev discussion about how to resolve this at

    http://mail.python.org/pipermail/python-dev/2010-March/098437.html

    @mdickinson
    Copy link
    Member

    Float-to-decimal comparisons have been fixed, and the Decimal hash function changed so that numerically equal Decimal and float instances hash equal, in r79583.

    The idea of raising an exception for float<->Decimal comparisons was discarded partly for backwards compatibility reasons, and partly because having __eq__ raise an exception causes difficulties for sets and dicts: for example, if equality checks between floats and Decimals raised an exception then '{-1.0, Decimal(-3)}' would give a valid set (the two values have different hashes, so the Decimal.__eq__ method is never invoked), but '{-1.0, Decimal(-2)}' would raise an exception (because the two set elements have equal hashes, so Decimal.__eq__ is invoked in order to determine whether the two elements are equal or not).

    (General principle: if x and y are hashable, x == y should never raise an exception.)

    This is still only a partial fix: comparisons between Decimal and Fraction instances still behave oddly. This seems less likely to cause problems in real-life code, though. The changes needed to make Decimal <-> Fraction comparisons correct are too intrusive and not yet well tested enough to make it into 2.7. (See bpo-8188).

    As discussed on python-dev, I'll forward port this to py3k; with any luck, py3k will also grow valid comparisons for Decimal <-> Fraction.

    @mdickinson
    Copy link
    Member

    Merged to py3k in r79668.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants