Opened 5 years ago

Closed 21 months ago

#20861 closed defect (duplicate)

numbers.Rational, numbers.Integer, etc. interfaces not implemented correctly

Reported by: dgulotta Owned by:
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: basic arithmetic Keywords: rational, numerator, denominator
Cc: jdemeyer, mcbell, slelievre Merged in:
Authors: Reviewers: Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The file numbers_abc.py registers some Sage classes as instances of numbers.Rational, numbers.Integer, etc. However, none of these classes implement the interfaces entirely correctly, because members that the interface expects to be properties (e. g. numerator and denominator in numbers.Rational) are instead functions.

Attempts to use these interfaces will fail in a way that may be confusing to the user:

sage: import fractions
sage: fractions.Fraction(1/2)
Fraction(<built-in method numerator of sage.rings.rational.Rational object at 0x7f4268425b30>, <built-in method denominator of sage.rings.rational.Rational object at 0x7f4268425b30>)
sage: fractions.Fraction(1,2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-cee53c78dd93> in <module>()
----> 1 fractions.Fraction(Integer(1),Integer(2))

/home/dan/SageMath/local/lib/python/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'

Change History (5)

comment:1 Changed 5 years ago by jdemeyer

Honestly, I don't think this is easy to fix. Sage has a history of using methods for these and related things. So either we drop internal consistency within Sage or we drop compatibility with the numbers ABC.

comment:2 Changed 21 months ago by slelievre

  • Cc jdemeyer mcbell slelievre added
  • Keywords rational numerator denominator added
  • Milestone changed from sage-7.3 to sage-duplicate/invalid/wontfix
  • Reviewers set to Samuel Lelièvre
  • Status changed from new to needs_review

Proposing to close as a duplicate of #28234 where a solution is being proposed.

comment:3 Changed 21 months 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 https://discuss.python.org/t/pep-3141-ratio-instead-of-numerator-denominator/2037/27?u=jdemeyer)

comment:4 follow-up: Changed 21 months ago by dgulotta

I filed the bug mainly because it seems like a bad idea to claim to implement interfaces that are not actually implemented. I probably should have been more clear about this, but I would consider just not registering the instances to be a solution to the problem.

comment:5 in reply to: ↑ 4 Changed 21 months ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from needs_review to closed

OK, thanks for that info, it's useful to know! I'm closing this now as duplicate of #28234.

Note: See TracTickets for help on using tickets.