numbers.Rational implements __float__ incorrectly

[wm75]

BPO	23975
Nosy	@pfmoore, @mdickinson, @wm75, @skirpichev
PRs	bpo-23975: Correct conversion of Rational’s to float #25619
Files	Rational.float.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 = None
closed_at = None
created_at = <Date 2015-04-16.20:06:57.803>
labels = ['type-bug', 'library']
title = 'numbers.Rational implements __float__ incorrectly'
updated_at = <Date 2021-04-26.04:22:28.952>
user = 'https://github.com/wm75'

bugs.python.org fields:

activity = <Date 2021-04-26.04:22:28.952>
actor = 'Sergey.Kirpichev'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2015-04-16.20:06:57.803>
creator = 'wolma'
dependencies = []
files = ['39217']
hgrepos = []
issue_num = 23975
keywords = ['patch']
message_count = 7.0
messages = ['241270', '241340', '241636', '241637', '241638', '241639', '242113']
nosy_count = 4.0
nosy_names = ['paul.moore', 'mark.dickinson', 'wolma', 'Sergey.Kirpichev']
pr_nums = ['25619']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23975'
versions = ['Python 3.5']

[wm75]

numbers.Rational defines __float__ like this:

def __float__(self):
    """float(self) = self.numerator / self.denominator

It's important that this conversion use the integer's "true"
division rather than casting one side to float before dividing
so that ratios of huge integers convert without overflowing.

"""
return self.numerator / self.denominator

This assumes that division of two Integral numbers returns a float, which the numbers ABC does not enforce in any way.
IMO, the only logical assumption is that division of any two Integral or Rational numbers gives a Real, for which the ABC guarantees a __float__ method in turn.
So I think Rational.__float__ should

return float(self.numerator / self.denominator)

[mdickinson]

What happens if self.numerator / self.denominator returns another Rational instance? If I create MyRational and MyInteger classes, such that the quotient of two MyInteger instances is a MyRational instance and the numerator and denominator of a MyRational instance are both MyInteger instances, this suggestion would lead to an infinite recursion. OTOH, I agree that something needs to be fixed here, since having __float__ return a MyRational instance is definitely wrong.

[wm75]

Good point.

If the numbers ABC guaranteed numerator and denominator to be Integral numbers, this could be solved by:

return float(int(self.numerator) / int(self.denominator))

but since both could be Rationals again that does not seem to be an option either.
What could be done is trying to multiply out the numerator and denominator pair until both *are* Integrals, like:

num = self.numerator
den = self.denominator
while not (isinstance(num, Integral) and isinstance(den, Integral)):
    num = num.numerator * den.denominator
    den = den.numerator * num.denominator
return float(int(num) / int(den))

Clearly that's more complicated, but, more importantly, has the disadvantage that the loop will run forever if the final numerator or denominator is not registered correctly in the numeric tower.
So some kind of check for this situation would be required, but I do not have an idea right now what that should look like.

[pfmoore]

Is it not reasonable to simply say that implementations of numbers.Rational which allow the numerator and denominator to have types for which true division doesn't return a float, have to provide their own implementation of __float__()?

It's certainly less convenient, and probably surprising for users, but the alternative is trying to work around broken integer types - after all numbers.Complex.__truediv__ says "Should promote to float when necessary" in the docstring, which to me says that a type where a/b doesn't return a float doesn't conform to the numeric tower.

[pfmoore]

Alternatively, return int(self.numerator) / int(self.denominator). After all, a fraction whose numerator can't be represented as a Python (unlimited precision) integer is a pretty odd sort of fraction...

[wm75]

Is it not reasonable to simply say that implementations of numbers.Rational which allow the numerator and denominator to have types for which true division doesn't return a float, have to provide their own implementation of __float__()?

Unfortunately, the Rational type cannot always know what its numerator or denominator supports.
Look at fractions.Fraction: its only requirement for numerator and denominator is that they both should be Rational instances.
So a hypothetical MyInt like Mark describes it could be perfectly acceptable for Fraction except that it would fail to convert it to float.

It's certainly less convenient, and probably surprising for users, but the alternative is trying to work around broken integer types - after all numbers.Complex.__truediv__ says "Should promote to float when necessary" in the docstring, which to me says that a type where a/b doesn't return a float doesn't conform to the numeric tower.

I do read this docstring differently. To me, it means should promote to float if there is no other way to express the result (like when your custom type system does not define its own Float type).
It would, in fact, be really bad for custom type implementors if they would be forced to leave their type system when doing divisions.

Alternatively, return int(self.numerator) / int(self.denominator). After all, a fraction whose numerator can't be represented as a Python (unlimited precision) integer is a pretty odd sort of fraction...

The problem here is that self.numerator and/or self.denominator could both be Rational instances again, which are not expressible as a Python integer and, thus, are not enforced to provide __int__ by the numbers ABC.

[wm75]

After considering this for a while, I think:

return float(self.numerator / self.denominator)

is the best solution:

• it is simple and works reasonably well as a default

• it fixes Rational.__float__ for cases, in which numerator / denominator returns a custom Real instance

• in the problematic scenario brought up by Mark, in which truediv of the numerator and denominator returns another Rational creating a potentially infinite recursion, a RuntimeError will be raised when the maximum recursion limit is reached. This is not an unheard of error to run into while trying to implement a custom numeric type and will provide reasonable (though not ideal) information about the problem.

• the workaround for the above is obvious: it is ok for self.numerator / self.denominator to return a Rational instance, but its type should overwrite Rational.__float__ to break the recursion. This could get documented in the docstring of Rational.__float__.

I've attached a tentative patch.

[hauntsaninja]

Thanks, looks like this has been completed