Opened 11 years ago

Last modified 11 years ago

#9652 closed defect

Unnecesary and buggy code in — at Initial Version

Reported by: mderickx Owned by: AlexGhitza
Priority: minor Milestone: sage-4.6
Component: algebra Keywords:
Cc: mstreng, lftabera Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


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.

Change History (0)

Note: See TracTickets for help on using tickets.