Opened 14 years ago

Closed 14 years ago

#1148 closed enhancement (fixed)

[with patch, with positive review] valuation doesn't work for rational numbers

Reported by: dmharvey Owned by: dmharvey
Priority: minor Milestone: sage-2.8.15
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

It would be nice if valuation(3/5, 5) returned -1, but it does this:

sage: valuation(3/5, 5)
---------------------------------------------------------------------------
<type 'exceptions.ZeroDivisionError'>     Traceback (most recent call last)

/Users/david/series/<ipython console> in <module>()

/Users/david/sage-2.8.12/local/lib/python2.5/site-packages/sage/rings/arith.py in valuation(m, p)
    425     r=0
    426     power=p
--> 427     while m%power==0:
    428         r += 1
    429         power *= p

/Users/david/series/rational.pyx in sage.rings.rational.Rational.__mod__()

/Users/david/series/integer.pyx in sage.rings.integer.Integer.inverse_mod()

<type 'exceptions.ZeroDivisionError'>: Inverse does not exist.

Attachments (2)

1148.hg (1.2 KB) - added by dmharvey 14 years ago.
1148-more.patch (1.2 KB) - added by robertwb 14 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 14 years ago by mabshoff

  • Milestone set to sage-2.8.13

comment:2 Changed 14 years ago by dmharvey

  • Owner changed from somebody to dmharvey
  • Status changed from new to assigned

Changed 14 years ago by dmharvey

comment:3 Changed 14 years ago by dmharvey

  • Summary changed from valuation doesn't work for rational numbers to [with patch] valuation doesn't work for rational numbers

fixed it

Changed 14 years ago by robertwb

comment:4 Changed 14 years ago by cwitty

  • Summary changed from [with patch] valuation doesn't work for rational numbers to [with patch, with positive review] valuation doesn't work for rational numbers

I tried the combination of 1148.hg and 1148-more.patch patch against 2.8.14. The source code looks reasonable, and doctesting arith.py and rational.pyx (the two files touched by the change) both succeed. (I did not try testall.)

In other words, looks good to me.

comment:5 Changed 14 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in 2.8.15.alpha1.

Note: See TracTickets for help on using tickets.