#9652: Unnecesary and buggy code in arith.py
------------------------+---------------------------------------------------
Reporter: mderickx | Owner: mderickx
Type: defect | Status: needs_review
Priority: minor | Milestone:
Component: algebra | Keywords:
Author: | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
------------------------+---------------------------------------------------
Changes (by lftabera):
* status: needs_info => needs_review
Old description:
> This code was added in #1148. I really think that the lines removed in my
> patch should be gone. The current (4.4.4) code is:
>
> {{{
> def valuation(m, p):
> if hasattr(m, 'valuation'):
> return m.valuation(p)
> if is_FractionFieldElement(m):
> return valuation(m.numerator()) - valuation(m.denominator())
> if m == 0:
> import sage.rings.all
> return sage.rings.all.infinity
> r = 0
> power = p
> while not (m % power): # m % power == 0
> r += 1
> power *= p
> return r
> }}}
>
> Putting implementation specific to Fraction fields in a global function
> is bad practice. And since fraction fields have an implementation of
> valuation part of the above code will not be execucted. If it magicaly
> get's excecuted it will return bad results since it doesn't take into
> account the input variable "p" as it should.
New description:
This code was added in #1148. I really think that the lines removed in my
patch should be gone. The current (4.4.4) code is:
{{{
def valuation(m, p):
if hasattr(m, 'valuation'):
return m.valuation(p)
if is_FractionFieldElement(m):
return valuation(m.numerator()) - valuation(m.denominator())
if m == 0:
import sage.rings.all
return sage.rings.all.infinity
r = 0
power = p
while not (m % power): # m % power == 0
r += 1
power *= p
return r
}}}
Putting implementation specific to Fraction fields in a global function is
bad practice. And since fraction fields have an implementation of
valuation part of the above code will not be execucted. If it magicaly
get's excecuted it will return bad results since it doesn't take into
account the input variable "p" as it should.
Patches to apply in order:
1. smallfix1-arith_valuation.2.3.patch
2. smallfix1-arith_valuation-doctest.2.3.patch
--
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9652#comment:28>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
