Opened 3 years ago

Last modified 7 weeks ago

#28234 needs_work defect

Sage Integers are incompatible with Python fraction.Fraction, statistics.mean, itertools.permutations

Reported by: mcbell Owned by:
Priority: major Milestone: sage-9.7
Component: basic arithmetic Keywords: days100
Cc: jdemeyer, mcbell, slelievre, vdelecroix, SimonKing, gh-posita, tscrim Merged in:
Authors: Mark Bell Reviewers:
Report Upstream: N/A Work issues:
Branch: public/28234 (Commits, GitHub, GitLab) Commit: 4977867e55d76c29b7286e2d542d8342889d8305
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

fraction.Fraction

In Sage 8.8 the code:

sage: from fractions import Fraction
sage: Fraction(1, 1)

raises the following error:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-0c4690dff1ca> in <module>()
----> 1 Fraction(Integer(1), Integer(1))

/Applications/SageMath-8.8.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'

This appears to be because of the fact that Python ints have numerator and denominator properties whereas for Sage integers these are methods. Therefore Fraction does not call these attributes leading to the crash

Note, turning off the pre-parser or doing:

sage: Fraction(int(1), int(1))
Fraction(1, 1)

works.

One solution to this would be to decorate sage.rings.integer.Integer.numerator() and sage.rings.integer.Integer.denominator() with the @property decorator, but this will likely cause a large number of systems to break.

Earlier report of this issue in #10928, comment 14 (a ticket about getitem of numpy matrix using Sage integers).

statistics.mean (see #29662)

Although the as_integer_ratio mechanism has been added to Python, ​statistics._exact_ratio does not use as_integer_ratio because it sees the Sage numbers' numerator and denominator methods and tries to use them as properties.

itertools.permutations

┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 9.5.beta7, Release Date: 2021-11-18               │
│ Using Python 3.9.9. Type "help()" for help.                        │
└────────────────────────────────────────────────────────────────────┘
sage: import itertools
sage: list(itertools.permutations(range(3),int(2)))
[(0, 1), (0, 2), (1, 0), (1, 2), (2, 0), (2, 1)]
sage: list(itertools.permutations(range(3),2))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-8-a9f97c1deeca> in <module>
----> 1 list(itertools.permutations(range(Integer(3)),Integer(2)))

TypeError: Expected int as r

see https://bugs.python.org/issue30537#msg295000

Attachments (1)

properties.patch (4.3 KB) - added by mcbell 3 years ago.
Patch to make numerator and denominator properties

Download all attachments as: .zip

Change History (69)

comment:1 Changed 3 years ago by mcbell

  • Description modified (diff)

comment:2 Changed 3 years ago by mcbell

  • Authors set to mcbell
  • Description modified (diff)

comment:3 Changed 3 years ago by mcbell

This issue also appears to affect Sage rationals where

sage: from fractions import Fraction
sage: Fraction(3/4)
Fraction(<built-in method numerator of sage.rings.rational.Rational object at 0x17d185ad0>, <built-in method denominator of sage.rings.rational.Rational object at 0x17d185ad0>)

since 3/4 is a sage Rational, which is a numbers.Rational and so during initialisation this block is triggered:

101         if denominator is None:
102             if isinstance(numerator, Rational):
103                 self._numerator = numerator.numerator
104                 self._denominator = numerator.denominator
105                 return self

which sets the numerator and denominator of the fraction to be the numerator and denominator methods of 3/4.

comment:4 Changed 3 years ago by slelievre

  • Authors mcbell deleted
  • Cc mcbell slelievre added

comment:5 Changed 3 years ago by mcbell

Note that the https://docs.python.org/3/library/numbers.html says that these should be properties:

class numbers.Rational
Subtypes Real and adds numerator and denominator properties, which should be in lowest terms. With these, it provides a default for float().

numerator
Abstract.

denominator
Abstract.

Similar is true for numbers.Integral

comment:6 Changed 3 years ago by slelievre

  • Cc jdemeyer vdelecroix added

Indeed, when Sage number classes were registered into Python numbers abstract base classes (#19571), it was overlooked that the Python numbers abc specifies rationals should have a numerator property and denominator property, while in Sage they are methods that one needs to call.

Some solutions discussed at Sage Days 100:

  • do nothing: then users must do different things for Sage numbers or Python numbers
  • change Sage, with a deprecation period and temporary fix where calling a Sage integer with no arguments returns this integer, with a warning
  • change Python

comment:7 Changed 3 years ago by mcbell

I have attached a potential patch for discussion which:

  1. Marks the numerator and denominator methods of Integer and Rational as properties using the property decorator
  2. Creates full numer and denom methods of Rational which return these properties
  3. Makes integers callable but:
    1. Raises a TypeError? (as before) if they are called with anything
    2. Generates a sage.misc.superseded.deprecation warning referring to this ticket and returns self if they are called without any arguments.

This means that if x = 7 then doing x.numerator returns 7 while doing x.numerator() is evaluated via 7() which, thanks to case 3b, generates a deprecation and then returns 7. A similar process happens if x = 7/3, although in this case no deprecation warning is raised when using the x.denom() which is provided as an alias.

Last edited 3 years ago by mcbell (previous) (diff)

Changed 3 years ago by mcbell

Patch to make numerator and denominator properties

comment:8 Changed 3 years ago by vdelecroix

  • Branch set to public/28234
  • Commit set to 6756542a807a5fc00252e4564ea4a550d67a1a45

New commits:

6756542make numerator/denominator properties

comment:9 follow-up: Changed 3 years ago by vdelecroix

a good point:

  • It indeed solves the problem and remains backward compatible

Some problems:

  • It affects a *lot* of files inside the Sage source code (at least 142 present doctest error) - this is easily solvable by patching the said files. But this kind of patch bomb might overlap with ongoing work of other people.
  • There are several other objects with a numerator/denominator method in Sage. Having integers and rationals behaving differently than the rest of Sage would be really weird. A solution would be: just do the same for other objects. But then the patch bomb becomes even bigger...

comment:10 in reply to: ↑ 9 Changed 3 years ago by mcbell

Replying to vdelecroix:

Some problems:

  • It affects a *lot* of files inside the Sage source code (at least 142 present doctest error) - this is easily solvable by patching the said files. But this kind of patch bomb might overlap with ongoing work of other people.

Regarding this, these appear to come in two flavours:

  1. The testing framework is not happy with deprecation warnings,
  2. This now makes the Sage Integer class callable and it appears that there are doctest that determine whether an object is an integer or not by testing whether it is callable!

While 1 could be solved by just removing the warning, 2 will involve making changes to hundreds of tests.

comment:12 Changed 3 years ago by SimonKing

  • Cc SimonKing added

comment:13 in reply to: ↑ 11 Changed 3 years ago by SimonKing

Replying to mcbell:

For an explicit example of the previous behaviour see: https://github.com/sagemath/sage/blob/master/src/sage/rings/polynomial/infinite_polynomial_element.py#L1523

Cool. I didn't recall that I had used callability at some point...

comment:14 follow-up: Changed 3 years ago by jdemeyer

I'm not sure that such a hack is worth it. We could at least ask Python upstream. One could argue that Python should use special methods for this like __numerator__.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by vdelecroix

Replying to jdemeyer:

I'm not sure that such a hack is worth it. We could at least ask Python upstream. One could argue that Python should use special methods for this like __numerator__.

It makes sense. Would you make such a proposal? Note that it affects Python but also numpy.

comment:16 Changed 3 years ago by mcbell

Another suggestion that could be considered, is to make numerator and denominator properties throughout Sage. Obviously such a change would not be backward compatible. Therefore it may have a major impact on external code that others have written - which must be taken into consideration.

However, within /src/sage/:

  1. There are only only 22 instances of "def numerator(" and 36 instances of "def denominator("
  2. In both cases, one is a function and the remainder are methods of classes. Of the latter, all-bar-two are properties (that is, their only argument is self)
  3. There are 482 uses of .numerator() and 650 uses of .denominator()
  4. Of these 31 and 31 are calls of the non-properties methods.

This suggest that 52 of these methods could have the @property decorators added relatively simply, and 4 exception methods will require some thought / care*. Similarly 1070 uses of these methods could be replaced via a direct substitution and 62 uses will require some care. So it appears that the effort required to implement within the Sage source, while large, would be relatively low as the majority of the work could be done with a find-and-replace.

*The exceptional methods are:

./src/sage/rings/continued_fraction.py:    def denominator(self, n):
./src/sage/symbolic/expression.pyx:    def denominator(self, bint normalize=True):

While the exceptional function that does not need to be changed is:

./src/sage/misc/functional.py:def denominator(x):

comment:17 Changed 3 years ago by mcbell

So having attempted the solution I proposed in comment 16 (via a small sed script) I do not think that this option will work. This is due to some of the .numerator() calls being calls on non-sage classes. For example, in rings/number_field/number_field_ideal.py the line

        return ZZ(q.numerator())

cannot be substitute since here q is a pari object and does not have a numer method.

comment:18 Changed 3 years ago by git

  • Commit changed from 6756542a807a5fc00252e4564ea4a550d67a1a45 to 70b1ecda035696998a057f381136af6b3a3f134a

Branch pushed to git repo; I updated commit sha1. New commits:

81b0862Attempting to make sure that callable things are not integers everywhere
9adc341Typo.
366ce6cComments to explain about the deprecation warning that could be added.
70b1ecdAlso dealing with hasattr(x, '__call__').

comment:19 follow-ups: Changed 3 years ago by mcbell

The above commits deals with the issues raised in comment 9. It does this by

  1. removing the deprecation warning - this could be added in in a later ticket and updating the relevant doctests to reflect this.
  2. Replacing all uses of hasattr(x, '__call__') with callable(x)
  3. Then replacing all uses of callable(x) with callable(x) and not isinstance(x, numbers.Integral)

This now means that the doctests pass and that doing

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

works.

Last edited 3 years ago by mcbell (previous) (diff)

comment:20 Changed 3 years ago by mcbell

  • Authors set to Mark Bell
  • Status changed from new to needs_review

comment:21 Changed 3 years ago by git

  • Commit changed from 70b1ecda035696998a057f381136af6b3a3f134a to 8f50595bd84c12bde0f35d0dd2529b49e62a072b

Branch pushed to git repo; I updated commit sha1. New commits:

8f50595Also dealing with isinstance(x, collections.Callable).

comment:22 Changed 3 years ago by git

  • Commit changed from 8f50595bd84c12bde0f35d0dd2529b49e62a072b to 4977867e55d76c29b7286e2d542d8342889d8305

Branch pushed to git repo; I updated commit sha1. New commits:

4977867Fixed tab character.

comment:25 Changed 3 years ago by jdemeyer

To the OP: why do you care about Python fractions? It's a serious question since Guido van Rossum seems quite negative about them (see the link at 23).

comment:26 in reply to: ↑ 19 ; follow-up: Changed 3 years ago by jdemeyer

Replying to mcbell:

  1. Replacing all uses of hasattr(x, '__call__') with callable(x)

This is independent of the issue at hand and should be done in a separate ticket.

comment:27 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by jdemeyer

Replying to vdelecroix:

Note that it affects Python but also numpy.

Please elaborate. Does numpy have fractions? I couldn't find anything like that.

comment:28 in reply to: ↑ 27 Changed 3 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

Note that it affects Python but also numpy.

Please elaborate. Does numpy have fractions? I couldn't find anything like that.

I meant that they are complient with the numbers abc protocol

>>> import numpy as np
>>> np.int16(123).numerator
123
>>> np.int16(123).denominator
1

comment:29 in reply to: ↑ 19 ; follow-up: Changed 3 years ago by jdemeyer

Replying to mcbell:

  1. Then replacing all uses of callable(x) with callable(x) and not isinstance(x, numbers.Integral)

I really don't like this change because it's an ugly hack and it may be bad for performance (since numbers.Integral is an abstract base class that arbitrary classes can register for, this is quite slow). Are those changes really needed?

comment:30 follow-up: Changed 3 years ago by jdemeyer

This is silly:

    def __call__(self, *args, **kwargs):
        if args or kwargs:
            raise TypeError("'{}.{}' object is not callable".format(self.__class__.__module__, self.__class__.__name__))

If you don't want to support *args or **kwargs, just write

    def __call__(self):
        return self

which is more efficient.

I also disagree with

        # If in the future we decide to deprecate this functionality then we can add:
        #   deprecation(28234, 'callable integers are only intended as a tempory patch until .numerator and .denominator properties are used everywhere')
        # This will require updating a large number of doctests to reflect this warning so we leave this for a future ticket.

I would rather keep the method as normal usage instead of deprecating it.

comment:31 Changed 3 years ago by jdemeyer

For the record, I'm not saying yet that I agree with this ticket. I'm just giving comments in case that we decide to go with this approach.

I certainly want to wait a bit to see where the Python discussion goes. It would help to have a convincing use case, instead of "I want to use fractions.Fraction in Sage just because".

comment:32 in reply to: ↑ 29 Changed 3 years ago by mcbell

Replying to jdemeyer:

Replying to mcbell:

  1. Then replacing all uses of callable(x) with callable(x) and not isinstance(x, numbers.Integral)

I really don't like this change because it's an ugly hack and it may be bad for performance (since numbers.Integral is an abstract base class that arbitrary classes can register for, this is quite slow). Are those changes really needed?

I agree that this is ugly and that it may impact performance. However it is necessary in at least some (in fact I believe a large percentage) of the callable(x) cases (for example, see ​https://github.com/sagemath/sage/blob/master/src/sage/rings/polynomial/infinite_polynomial_element.py#L1523). Without these the route through the code changes and so if they are omitted then the code raises an error. The tricky thing here is that (like in the above example) the error is often raised far away from where the callable(x) occurred, making these hard to track down.

It may be possible to remove some of these uses when you explicitly know that x is never a Sage Integer. However I would be extremely worried about these cases since if you are wrong then it is possible that the code does not raise an error but may compute a incorrect result. Now hopefully there would be doctests that would run all of these different possibilities but this may be a situation that people aren't expecting and so I'm not sure that we can rely on the doctests to save us.

Therefore I would strongly argue that these changes are needed everywhere. In fact, even in the few cases where it is completely obvious that x cannot be a Sage Integer, it may be better to keep these purely for consistency.

comment:33 in reply to: ↑ 30 Changed 3 years ago by mcbell

Replying to jdemeyer:

If you don't want to support *args or **kwargs, just write

    def __call__(self):
        return self

which is more efficient.

I agree that this is a better way to go initially. Thank you for pointing this out.

comment:34 follow-up: Changed 3 years ago by jdemeyer

The fact that making Sage integers callable breaks stuff is bad. So we should consider the pros and cons of this approach. I'm not sufficiently convinced about the pros (as Guido van Rossum said: what real world problems would this solve?). Note that the submitter at #20861 admitted to not really caring about fractions, so for the moment my feeling goes towards rejecting this fix.

comment:35 Changed 3 years ago by embray

I do find it a shame, in general, that there are so many methods on classes in Sage that should really be properties. I understand the reason ("what if you do want to add some optional arguments to that property later?"). But in that case I think there should be a separate method from the argument-less property.

It's also an unfortunate shortcoming in Python that there's know way to easily know ahead of time that when an attribute is looked up on an object, it will be subsequently called as in method call syntax. So there is no opportunity to return a different type for attribute access when doing something like a.property versus a.property(). It's fundamentally baked into Python's design that these cases can't be distinguished in some way.

I've thought in the past of working around this limitation by coming up with a generic way of proxying arbitrary types in such a way as to make them callable. So rather than making Integer callable as this ticket proposes, something like Rational.numerator would instead return an Integer instance wrapped in a proxy, which when called does something special with that instance, likely with reference to the original Rational that returned it (in this case the call method on the proxy would be trivial--it would just return the Integer itself).

Unfortunately that's a whole other can of worms itself, especially when you want to add a callable-proxy to types that are already callable in the first place.

comment:36 follow-up: Changed 3 years ago by embray

Though I did just see Jeroen's point here that technically one could implement specialized method call syntax. But it's still really confusing. For example you'd have a.foo() do something different from x = a.foo; x(). And yes, it would be very CPython specific--but maybe that's okay, for special cases in Sage, to take advantage of?

Another super hacky workaround I've considered in the past for this (which would only work in CPython with pure Python code) would be to have a special kind of descriptor that actually looks ahead in the bytecode to see if (within some reasonable localization) the object returned by the descriptor will be called, and thus allow different behavior in that case. Again, a hugely problematic hack, but possibly "good enough" for some common cases?

comment:37 in reply to: ↑ 36 Changed 3 years ago by jdemeyer

Replying to embray:

Though I did just see Jeroen's point here that technically one could implement specialized method call syntax. But it's still really confusing. For example you'd have a.foo() do something different from x = a.foo; x().

To expand on this, this is part of the work on PEP 590. Note the different bytecode generated:

>>> from dis import dis
>>> def f(obj):
...     return obj.meth()
>>> def g(obj):
...     x = obj.meth
...     return x()
>>> dis(f)
  2           0 LOAD_FAST                0 (obj)
              2 LOAD_METHOD              0 (meth)
              4 CALL_METHOD              0
              6 RETURN_VALUE                                                                                                                                            
>>> dis(g)                                                                                                                                                             
  2           0 LOAD_FAST                0 (obj)                                                                                                                        
              2 LOAD_ATTR                0 (meth)                                                                                                                       
              4 STORE_FAST               1 (x)

  3           6 LOAD_FAST                1 (x)
              8 CALL_FUNCTION            0
             10 RETURN_VALUE

LOAD_METHOD and CALL_METHOD are an optimization to avoid creating a temporary bound method object when executing obj.meth(). It basically turns the call into a call of the unbound method. This optimization used to be enabled only for two specific method classes (one for Python methods and one for C methods). But PEP 590 allows this optimization for arbitrary classes. In theory, it should just be an optimization not affecting the actual behaviour. But it does allow doing arbitrary hacky stuff when LOAD_METHOD/CALL_METHOD is used.

And yes, it would be very CPython specific--but maybe that's okay, for special cases in Sage, to take advantage of?

We need to make sure that Cython supports it too. Since PEP 590 is so new, it's not in the current stable release. But the big upcoming Cython 3.0 should at least partially support it.

comment:38 Changed 3 years ago by jdemeyer

I forgot to mention that PEP 590 needs Python 3.8, for which there is no stable release yet.

comment:39 follow-up: Changed 3 years ago by embray

How does it know to generate special bytecode in this case? Is it just hacked into the parser somehow?

Part of me really does think there should be a way to exploit direct method calls as a special case, even though it would be a slightly confusing inconsistency.

comment:40 in reply to: ↑ 34 ; follow-up: Changed 3 years ago by slelievre

Replying to jdemeyer:

as Guido van Rossum said: what real world problems would this solve?

Some mathematical packages for dynamics, geometry and topology of surfaces and 3-manifolds, such as Curver, Flipper, SnapPy, Twister, Veerer, can be run either with pure Python, or with Sage. Some of them use the Fraction constructor from Python's fractions module, which works with Python ints or rationals and fails with Sage integers or rationals. That's the real-world problem.

comment:41 in reply to: ↑ 39 Changed 3 years ago by jdemeyer

Replying to embray:

Is it just hacked into the parser somehow?

Yes. When the parser sees something of the form obj.attr(), it uses those special bytecodes. Further details in https://bugs.python.org/issue26110

comment:42 in reply to: ↑ 40 ; follow-up: Changed 3 years ago by jdemeyer

Replying to slelievre:

Some mathematical packages for dynamics, geometry and topology of surfaces and 3-manifolds, such as Curver, Flipper, SnapPy, Twister, Veerer, can be run either with pure Python, or with Sage. Some of them use the Fraction constructor from Python's fractions module

OK, I quickly looked at these packages. First of all, I couldn't find a package called "Veerer".

Two don't use fractions:

Two do use fractions:

However, it's not obvious to me how Sage integers or rationals could end up as an argument to Fraction in these packages. curver doesn't use Sage at all. snappy does use Sage (for example, for Gröbner basis computations), but it seems that output from Sage is converted to snappy data structures, so it doesn't really do computations with Sage data types.

comment:43 follow-up: Changed 3 years ago by vdelecroix

flipper does indirectly uses Fraction through realalg. And it is easy to break

sage: import realalg
sage: realalg.RealNumberField([1,0,-2])
Traceback (most recent call last):
...
TypeError: invalid input: <built-in method numerator of sage.rings.integer.Integer object at 0x7fca4682fd20>

I thought about providing support for sage Rational in realalg... but it is a can of worms.

comment:44 in reply to: ↑ 43 ; follow-up: Changed 3 years ago by jdemeyer

Replying to vdelecroix:

but it is a can of worms.

A can of worms is precisely what we're looking for here. Guido claims that there is no problem, so we some data to prove him wrong.

comment:45 in reply to: ↑ 44 Changed 3 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

but it is a can of worms.

A can of worms is precisely what we're looking for here. Guido claims that there is no problem, so we some data to prove him wrong.

Agreed.

As we make the PEP idea better, we should also think about making Rational an easy drop-in replacement for Fraction. In other words, the following kind of thing should work

try:
    from sage.rings.rational import Rational as Fraction
except ImportError:
    try:
        from gmpy2 import mpq as Fraction
    except ImportError:
        from fractions import Fraction

I see a serious obstruction for now: the constructors are not compatible.

sage: from fractions import Fraction
sage: from gmpy2 import mpq
sage: Fraction(2r, 3r)
Fraction(2, 3)
sage: mpq(2,3)
mpq(2,3)
sage: Rational(2,3)   # second argument is "base"
2

comment:46 in reply to: ↑ 42 Changed 3 years ago by mcbell

Replying to jdemeyer:

Two do use fractions:

However, it's not obvious to me how Sage integers or rationals could end up as an argument to Fraction in these packages. curver doesn't use Sage at all.

Sage Integers can end up in Fractions whenever a user uses these packages interactively or from within a sage script / notebook. For example:

sage: import curver
sage: curver.load(2, 1).triangulation([0, 99, 99, 1, 100, 0, 0, 0, 0])

crashes since the values that are passed to the functions within curver are sage Integers. This happens even though curver has scattered throughout it lines (equivalent to):

    assert isinstance(n, numbers.Integral)

but since Sage Integers have been registered as Integral these catches do not work.

Of course, doing

sage: curver.load(2, 1).triangulation([0r, 99r, 99r, 1r, 100r, 0r, 0r, 0r, 0r])

works without an issue.

comment:47 Changed 3 years ago by mcbell

Personally, I am more worried about code that does incorrect / dangerous things when it encounters. For example, I could imagine something like:

def prod_numerator_denominator(x):
    ''' Since x = p / q (where q is possibly 1), return p * q. '''
    try:
        # If x is Rational then this will work.
        return x.numerator * x.denominator
    except:
        # x was not a Rational, so we can take q == 1 and just return x
        return x

assert prod_numerator_denominator(Fraction(3r, 2r)) == 6
assert prod_numerator_denominator(3) == 3
assert prod_numerator_denominator(3 / 2) == 6  # Fails since prod_numerator_denominator(3 / 2) returns 3

Or even

def f(x):
    if not isinstance(x, numbers.Rational):
        return NotImplemented

    # So now I know x is a Rational and so implements .numerator and .denominator properties
    try:
        return x.numerator + x.denominator
    except:
        # I know the code here can never be run so I can it should be safe to put something bad here
        import os
        os.remove('/')
Last edited 3 years ago by mcbell (previous) (diff)

comment:48 Changed 3 years ago by embray

To me an obvious answer here is for Sage Rationals and Integers to just get some kind of .as_pyfraction() method to convert them to a Fraction.

It's not ideal of course. One would really prefer to see it just work, transparently. But when interfaces collide the only recourse, if you can't easily change the interfaces, is to make some kind of adapter.

I liked Jeroen's __ratio__ proposal and thought it made a lot of sense. But it seems it wasn't too popular. And even if it did happen one day we'd have to wait for the change in Python (or monkey-patch the fractions module which I suppose is another possibility...)

comment:49 follow-up: Changed 3 years ago by slelievre

I often wished there was a.num_denom() to get a.numerator() and a.denominator() at once, the same way a.quo_rem(b) gives the quotient a // b and the remainder a % b at once.

comment:50 in reply to: ↑ 49 Changed 3 years ago by jdemeyer

Replying to slelievre:

I often wished there was a.num_denom()

It's called .as_integer_ratio() and it will be supported in Python 3.8 for all standard numeric types (int, float, decimal.Decimal and fractions.Fraction). Clearly, this is something that Sage types should also have.

This doesn't yet solve the problem that this ticket wants to solve, but it's getting closer.

comment:51 Changed 3 years ago by slelievre

Okay, I opened #28348 for that.

comment:52 Changed 3 years ago by jdemeyer

I opened #28347 for that, needs review.

comment:53 Changed 2 years ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:54 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

comment:55 Changed 22 months ago by mkoeppe

  • Status changed from needs_review to needs_work

merge conflict

comment:56 Changed 21 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:57 Changed 16 months ago by embray

Any interest in continuing to work on this? I've just been bitten by this issue in my own code.

comment:58 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:59 Changed 14 months ago by slelievre

  • Description modified (diff)

This issue was already reported in #10928, comment 14 (a ticket about getitem of numpy matrix using Sage integers).

Hopefully with #28347 in, things are easier than back at Sage Days 100?

comment:60 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:61 Changed 10 months ago by gh-posita

  • Cc gh-posita added

comment:62 Changed 9 months ago by mkoeppe

This issue also shows up when trying to use the statistics module with Sage integers or rationals, see #29662.

Although the as_integer_ratio mechanism has been added to Python, statistics._exact_ratio does not use as_integer_ratio because it sees the Sage numbers' numerator and denominator methods and tries to use them as properties.

comment:63 Changed 9 months ago by mkoeppe

  • Summary changed from Sage Integers are incompatible with Python Fractions to Sage Integers are incompatible with Python Fractions and statistics.mean etc.

comment:64 Changed 5 months ago by mkoeppe

  • Cc tscrim added
  • Description modified (diff)
  • Summary changed from Sage Integers are incompatible with Python Fractions and statistics.mean etc. to Sage Integers are incompatible with Python fraction.Fraction, statistics.mean, itertools.permutations

comment:65 Changed 5 months ago by mkoeppe

  • Description modified (diff)

comment:66 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:67 in reply to: ↑ 26 Changed 3 months ago by jhpalmieri

Replying to jdemeyer:

Replying to mcbell:

  1. Replacing all uses of hasattr(x, '__call__') with callable(x)

This is independent of the issue at hand and should be done in a separate ticket.

Done in #33451.

comment:68 Changed 7 weeks ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7
Note: See TracTickets for help on using tickets.