[issue23975] numbers.Rational implements __float__ incorrectly

[Sergey B Kirpichev]

Change by Sergey B Kirpichev :


--
pull_requests: +24321
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/25619

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Sergey B Kirpichev]

Change by Sergey B Kirpichev :


--
nosy: +Sergey.Kirpichev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Wolfgang Maier]

Wolfgang Maier added the comment:

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.

--
keywords: +patch
Added file: http://bugs.python.org/file39217/Rational.__float__.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Paul Moore]

Paul Moore added the comment:

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...

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Paul Moore]

Paul Moore added the comment:

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.

--
nosy: +paul.moore

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Wolfgang Maier]

Wolfgang Maier added the comment:

 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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Wolfgang Maier]

Wolfgang Maier added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Mark Dickinson]

Mark Dickinson added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[Wolfgang Maier]

New submission from Wolfgang Maier:

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)

--
components: Library (Lib)
messages: 241270
nosy: wolma
priority: normal
severity: normal
status: open
title: numbers.Rational implements __float__ incorrectly
type: behavior
versions: Python 3.5

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23975] numbers.Rational implements __float__ incorrectly

[R. David Murray]

Changes by R. David Murray rdmur...@bitdance.com:


--
nosy: +mark.dickinson

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23975
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com