Opened 9 years ago

Closed 9 years ago

#14870 closed defect (fixed)

Failure with Python int modulo a rational

Reported by: tscrim Owned by: tscrim
Priority: critical Milestone: sage-5.12
Component: basic arithmetic Keywords:
Cc: vbraun, roed Merged in: sage-5.12.beta0
Authors: Travis Scrimshaw Reviewers: Beth Malmskog
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


Some fun I came across:

sage: int(5) % QQ(2)
TypeError                                 Traceback (most recent call last)
<ipython-input-4-8ec0f9994ded> in <module>()
----> 1 int(Integer(5)) % QQ(Integer(2))

TypeError: Argument 'self' has incorrect type (expected sage.rings.rational.Rational, got int)

Note that this works:

sage: 5 % QQ(2)

Attachments (1)

trac_14870-fix_int_mod_QQ-ts.patch (1.7 KB) - added by tscrim 9 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Cc vbruan roed added
  • Status changed from new to needs_review

Fixed. Thanks Volker and David for your insight.

comment:2 Changed 9 years ago by tscrim

  • Cc vbraun added; vbruan removed

comment:3 Changed 9 years ago by vbraun

You shouldn't check the type exactly, derived classes of rational should be treated like a rational.

Also, the whole PY_* macro usage that you see in the sources is mostly historical, nowadays Cython understands isinstance(x, Rational) just fine and this is the preferred form of a type check.

comment:4 Changed 9 years ago by malmskog

  • Status changed from needs_review to positive_review

We tried using decimals like 3.0, coerced rationals like QQ(3), non-reduced fractions like 4/2, complex-like values like 3+0*I, and absolute values of integers as moduli (second argument in function)--all of these were correctly interpreted as integers. We also found correct error messages for moduli that didn't make sense, for example mod(31/10, 10) produces a correct error message. Looks good!

Changed 9 years ago by tscrim

comment:5 Changed 9 years ago by tscrim

New version as per Volker's comment.

comment:6 Changed 9 years ago by tscrim

@malmskog, you'll need to write your real name as the reviewer.

comment:7 Changed 9 years ago by malmskog

  • Reviewers set to Beth Malmskog

comment:8 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-5.11

comment:9 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:10 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.