Making Integer and Rational compatible with Python Fraction

[Mark Bell]

During Sage Days 100, I experienced an issue with Sage's Integers and Rational not being compatible with Python's built in Fraction class. This made me unable to run my Python code within Sage and so I raised ticket 28234. For example, in Sage 8.8:

sage: from fractions import Fraction
sage: Fraction(7, 3)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-4a5c214ab004> in <module>()
----> 1 Fraction(Integer(7), Integer(3))


/Applications/SageMath-8.6.app/Contents/Resources/sage/local/lib/python2.7/fractions.pyc in __new__(cls, numerator, denominator)
    152             isinstance(denominator, Rational)):
    153             numerator, denominator = (
--> 154                 numerator.numerator * denominator.denominator,
    155                 denominator.numerator * numerator.denominator
    156                 )


TypeError: unsupported operand type(s) for *: 'builtin_function_or_method' and 'builtin_function_or_method'
sage: Fraction(7)
Fraction(<built-in method numerator of sage.rings.integer.Integer object at 0x182ab93f0>, <built-in method denominator of sage.rings.integer.Integer object at 0x182ab93f0>)
sage: Fraction(7 / 3)
Fraction(<built-in method numerator of sage.rings.rational.Rational object at 0x182ab2950>, <built-in method denominator of sage.rings.rational.Rational object at 0x182ab2950>)

This issue is because, despite the Integer and Rational class being registered as Python's numbers.Integral and numbers.Rational classes, they do not implement numerator and denominator properties as required by the Python specification. Instead they implement numerator and denominator methods. This issue has been spotted a number of times [1] [2].

During the course of the workshop several solutions were suggested and investigated including:

1. Make Integer and Rational numerator and denominator properties and accept that this would not be backwards compatible within Sage
2. Make Integer and Rational numerator and denominator properties and add numer and denom methods to classes that have these properties and switch to using these throughout
3. Make Integer and Rational  numerator and denominator properties and make Integers callable and return themselves
4. Raise a PEP to change the Python specification to be / include numerator and denominator methods
5. Do not register Integer and Rational as numbers.Integral and numbers.Rational
6. Do nothing and so accept that Sage does not perfectly match the Python specification

As part of the workshop, I worked on implementing approach #3 within branch 28234. This implementation is now complete and passes all of Sages doctests [although its implementation revealed a bug in sagemath-patchbot which I subsequently fixed]. This means that now if, for example, x = 5 then both x.numerator and x.numerator() return 5 [in the latter case x.numerator returns 5, and then 5() returns 5]. Hence not only does this make Sage Integers and Rationals compatible with Python Fractions:

sage: from fractions import Fraction
sage: Fraction(7, 3)
Fraction(7, 3)
sage: Fraction(7)
Fraction(7, 1)
sage: Fraction(7/3)
Fraction(7, 3)

but this change is morally backwards compatible with existing Sage code.

However, this approach makes Sage Integers callable and so additional work was needed to ensure that the control flow within Sage kernel was not affected. I used a short Python script to generate a patch (that was applied within branch 28234) that:

1. Replaced all uses of hasattr(x, '__call__') with callable(x)
2. Replaced all uses of isinstance(x, collections.Callable) with callable(x)
3. Replaced all uses of callable(x) with callable(x) and not isinstance(x, numbers.Integral)

Therefore if this branch is merged then users may need to make similar changes to their code if they ever rely on the fact that Integers were not callable.

Now regarding the other approaches:

• Approach #1 would be backwards incompatibly and would likely affect a substantially larger number of users (anyone who has ever access a numerator). It would also mean that care would be needed as to use x.numerator or x.numerator() depending on the type of x.
• Approach #2 was attempted but presented difficulties since numer() methods would also need to be added all external objects (e.g. Pari object) that are available within Sage that implement numerator() methods.
• Approach #4 was also started and discussion about changing the Python specification from PEP 3141 can be found here. This is likely to also have a knock-on effect on other systems such as numpy.
• Approach #5 would also result in serious backwards compatibility issues and would still mean that Sage Integers and Rationals would not be compatible with Python Fractions.

Therefore I would like to ask the Sage development community for their opinions on:

• the different approaches and whether approach #3 is the most suitable
• whether branch public/28234 is the most suitable implementation of approach #3 and whether it should be merged into Sage
• whether similar changes should be made to other classes within Sage that implement numerator / denominator methods
• whether similar changes should be made to the .real() and .imag() methods which should also be properties under the Python specification

I would also like to emphasise that although only these six approaches were discussed during the workshop, there may well be others. Therefore others should be encouraged to alternative approaches so that these can all be discussed together.

Regards,

Mark Bell

[Simon King]

Hi!

If we say that Sage Integers and Rationals should comply with Python's
requirements, then for consistency, the same should hold for elements of
integral domains respectively of their fraction fields.

In particular, the required element methods of QuotientFields() should
be removed and instead one should require numerator and denominator
element proporties (if that's possible) for IntegralDomains().

Inside the Sage library, either solution is doable with a different
amount of effort. The questions are:
- To what extent each solution breaks external code?
- Are these backwards incompatibilities acceptable?

Currently a modification of Mark's suggestion 3. (perhaps including 2.)
makes most sense to me:
- Change numerator and denominator of *all* integral domain elements
(not just integers and rationals) into properties, also taking into
account the stuff required by categories.
- Make integers and rationals callable, returning themselves and
raising a deprecation warning.
- (Perhaps) add numer and denom methods.

Rationale: In external code, I guess the most common use case of
numerator and denominator methods is for rationals. Hence, the damage to
external code would be minimised by preserving backwards compatibility
for Rationals. And after some deprecation period we would achieve a
consistent behaviour for all kindes of fraction field elements compliant
with python numbers.

Best regards,
Simon

[Jeroen Demeyer]

On 2019-07-27 18:49, 'Mark Bell' via sage-devel wrote:
> During Sage Days 100, I experienced an issue with Sage's Integers and
> Rational not being compatible with Python's built in Fraction class.
> This made me unable to run my Python code within Sage

It would be good to explain better exactly what kind of code you were
running and why this is an issue for you.

I mentioned this issue to the Python devs and Guido van Rossum basically
rejected the problem. He literally said "what real-world problem does
that solve?" when talking about letting Python fractions interoperate
with Sage fractions, see
https://discuss.python.org/t/pep-3141-ratio-instead-of-numerator-denominator/2037/27?u=jdemeyer

If this would be a simple bugfix, I would dismiss Guido's comment as
irrelevant. But in this case, there is no easy fix. Every proposed fix
is somewhat backwards incompatible and can potentially break stuff. So
we should only apply a fix if it will actually solve a real problem that
people are encountering.

Jeroen.

[TB]

On 05/08/2019 0:04, Jeroen Demeyer wrote:
> On 2019-07-27 18:49, 'Mark Bell' via sage-devel wrote:
>> During Sage Days 100, I experienced an issue with Sage's Integers and
>> Rational not being compatible with Python's built in Fraction class.
>> This made me unable to run my Python code within Sage
>
> It would be good to explain better exactly what kind of code you were
> running and why this is an issue for you.
>
> I mentioned this issue to the Python devs and Guido van Rossum basically
> rejected the problem. He literally said "what real-world problem does
> that solve?" when talking about letting Python fractions interoperate
> with Sage fractions, see
> https://discuss.python.org/t/pep-3141-ratio-instead-of-numerator-denominator/2037/27?u=jdemeyer
>

Not with Fraction, but I think I encountered something similar with
num.real() and num.imag(). IIRC, it was for plotting a simple function
that used either the methods real() and imag() or the properties, and it
obviously required a fix. Something like:

sage: plot(lambda x: x.imag + 2) # Works
sage: plot(lambda x: x.imag() + 2, (x, -3, 3)) # Empty plot

In other words, I agree with Vincent at
https://discuss.python.org/t/pep-3141-ratio-instead-of-numerator-denominator/2037/6
To add another data point about triggering unexpected computations,
numpy.array does have .real and .imag properties.

>
> If this would be a simple bugfix, I would dismiss Guido's comment as
> irrelevant. But in this case, there is no easy fix. Every proposed fix
> is somewhat backwards incompatible and can potentially break stuff. So
> we should only apply a fix if it will actually solve a real problem that
> people are encountering.

Regards,
TB

[Jeroen Demeyer]

On 2019-08-06 17:30, TB wrote:
> sage: plot(lambda x: x.imag + 2) # Works
> sage: plot(lambda x: x.imag() + 2, (x, -3, 3)) # Empty plot

That's "only" an annoyance: you need to use x.imag or x.imag() depending
on what x is. That's not a problem of interoperability. And if you
really want generic code, you can always write complex(x).imag (at some
performance cost). So it's less a problem than Fraction.

> To add another data point about triggering unexpected computations,
> numpy.array does have .real and .imag properties.

As far as I can see, it doesn't actually use .real or .imag on the
objects that it wraps, so again there is no problem here.

[E. Madison Bray]

What if, at some point, we just make a Sage release that breaks
backwards compatibility? It doesn't have to be as huge a breakage as
Python 2 -> Python 3. But sometimes it's just necessary to bite the
bullet and break things. That's what version numbering is for.

It would help, of course, if every current Sage release weren't an
arbitrary mix of enhancements and bug fixes.

[Jeroen Demeyer]

On 2019-08-07 15:42, E. Madison Bray wrote:
> What if, at some point, we just make a Sage release that breaks
> backwards compatibility?

We could, but I don't think that PEP 3141 is worth breaking backwards
compatibility for.

[E. Madison Bray]

I'm inclined to agree, unfortunately.