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

Decimal.from_float works incorrectly for non-binary floats #71162

Closed
serhiy-storchaka opened this issue May 8, 2016 · 8 comments
Closed

Decimal.from_float works incorrectly for non-binary floats #71162

serhiy-storchaka opened this issue May 8, 2016 · 8 comments
Labels
extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 26975
Nosy @rhettinger, @facundobatista, @mdickinson, @skrah, @serhiy-storchaka

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 = None
closed_at = <Date 2016-06-21.21:58:01.352>
created_at = <Date 2016-05-08.06:45:45.384>
labels = ['extension-modules', 'invalid', 'type-bug', 'library']
title = 'Decimal.from_float works incorrectly for non-binary floats'
updated_at = <Date 2016-06-21.21:58:01.350>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-06-21.21:58:01.350>
actor = 'skrah'
assignee = 'none'
closed = True
closed_date = <Date 2016-06-21.21:58:01.352>
closer = 'skrah'
components = ['Extension Modules', 'Library (Lib)']
creation = <Date 2016-05-08.06:45:45.384>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 26975
keywords = []
message_count = 8.0
messages = ['265116', '265118', '265123', '265138', '265140', '265142', '269029', '269030']
nosy_count = 5.0
nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'skrah', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26975'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

@serhiy-storchaka
Copy link
Member Author

Decimal.from_float() works correctly only if the denominator of as_integer_ratio() of argument is the power of two. Example:

>>> from decimal import Decimal
>>> class DecimalFloat(float):
...     def as_integer_ratio(self):
...         return Decimal(str(self)).as_integer_ratio()
...     def __abs__(self):
...         return self
... 
>>> DecimalFloat(1.2).as_integer_ratio()
(6, 5)
>>> Decimal.from_float(DecimalFloat(1.2))
Decimal('1.50')

@serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 8, 2016
@mdickinson
Copy link
Member

True. Though having a subclass of float that overrides as_integer_ratio seems a fairly unlikely use-case. We could add a check for subclasses of float that the denominator is a power of 2 (using the usual trick: a positive integer n is a power of 2 if and only if n & (n-1) is zero) and raise a suitable error otherwise.

I doubt it's worth trying to support arbitrary return values from as_integer_ratio. Note that by overriding as_integer_ratio, you're breaking its "contract": the docs say

Return a pair of integers, whose ratio is exactly equal to the original
float and with a positive denominator.

and you've lost that "exactly equal". I think it's reasonably to get wrong results or an exception in that case.

@skrah
Copy link
Mannequin

skrah mannequin commented May 8, 2016

It seems to work correctly here for non-binary floats:

>>> from _pydecimal import Decimal
>>> Decimal.from_float(Decimal("1.2"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.6/_pydecimal.py", line 739, in from_float
    raise TypeError("argument must be int or float.")
TypeError: argument must be int or float.

I think we should not support strange inheritance hierarchies that
break the expected return values of parent class methods.

@serhiy-storchaka
Copy link
Member Author

Once I was going to propose to make as_integer_ratio() returning a pair (n, d) with minimal positive d that n/d (as float) is exactly equal to the original float. I think this is legitimate implementation and doesn't contradict the documentation.

I see following ways to resolve this issue:

  1. Ignore it.

  2. Ignore methods overriding:
    2a) either convert float subclass to exact float with known behavior;
    2b) or directly call static float.__abs__() and float.as_integer_ratio() instead of resolving dynamic methods.

  3. Check that the denominator is a power of 2 and raise error otherwise.

  4. Implement different different algorithm. Returns decimal n / 10**k with minimal k that after converting to float is exactly equal to the original float.

Most of these solutions will automatically fix bpo-26974, so it doesn't need special fix.

@skrah
Copy link
Mannequin

skrah mannequin commented May 8, 2016

As Mark hinted at, many people would say there is no issue at all.
Subclassing like that often breaks the Liskov Substitution Principle.

For more information, see e.g.:

http://okmij.org/ftp/Computation/Subtyping/

"Alas, LSP when considered from an OOP point of view is undecidable. You cannot count on a compiler for help in pointing out an error. You cannot rely on regression tests either. It's manual work -- you have to see the problem"

@mdickinson
Copy link
Member

I see following ways to resolve this issue:

I don't think 4. is an option: for input of exact type float, from_float should continue to produce the exact value of the float, rather than an approximation to that exact value. There's code out there that depends on this behaviour. Options 1 through 3 all seem reasonable to me.

@rhettinger
Copy link
Contributor

I vote for ignoring this and calling it not-a-bug. In a way, it is no more interesting than a dict like object defining __hash__ to return a random number.

The intention for from_float() is to convert binary floats. It was not meant to be generalized to handle arbitrary fractions. I concur with Mark's comment, "Note that by overriding as_integer_ratio, you're breaking its "contract"".

@skrah
Copy link
Mannequin

skrah mannequin commented Jun 21, 2016

Let's close it then.

@skrah skrah mannequin closed this as completed Jun 21, 2016
@skrah skrah mannequin added the invalid label Jun 21, 2016
@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
extension-modules C modules in the Modules dir 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