Sage: Ticket #9652: Unnecesary and buggy code in arith.py
https://trac.sagemath.org/ticket/9652
<p>
This code was added in <a class="closed ticket" href="https://trac.sagemath.org/ticket/1148" title="enhancement: [with patch, with positive review] valuation doesn't work for rational ... (closed: fixed)">#1148</a>. I really think that the lines removed in my patch should be gone. The current (4.4.4) code is:
</p>
<pre class="wiki">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
</pre><p>
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.
</p>
<p>
Patches to apply in order:
</p>
<ol><li>smallfix1-arith_valuation.2.3.patch
</li></ol><ol start="2"><li>smallfix1-arith_valuation-doctest.2.3.patch
</li></ol>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/9652
Trac 1.1.6mderickxSat, 31 Jul 2010 20:39:32 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:1
https://trac.sagemath.org/ticket/9652#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketmstrengMon, 02 Aug 2010 14:39:57 GMT
https://trac.sagemath.org/ticket/9652#comment:2
https://trac.sagemath.org/ticket/9652#comment:2
<p>
Two questions:
</p>
<ul><li>Is there any case in which any code after <code>return infinity</code> is executed and succesful?
</li><li>Is there any case in which any code after <code>return m.valuation(...)</code> is executed and useful?
</li></ul><p>
Can't you remove a lot more while you are at it? The code seems to be there to catch off weird unexpected cases that don't have a valuation attribute, but
</p>
<ol><li>if the case is weird enough, then nothing guarantees that this function terminates
</li><li>if the case happens to have a negative valuation, then the function returns 0 instead of a negative number
</li></ol><p>
While you are at it, I found that the function doesn't handle the valuation attribute of power series well (because it doesn't allow you to specify the (unique) prime p).
</p>
<pre class="wiki">x = QQ[['x']].gen()
valuation(x^2, x)
TypeError: valuation() takes no arguments (1 given)
valuation(x^2)
TypeError: valuation() takes exactly 2 arguments (1 given)
</pre><p>
The first error is because <code>PowerSeries.valuation()</code> takes no arguments; the second is because the global function takes exactly 2 arguments.
</p>
<p>
How about defining the global function simply as follows?
</p>
<pre class="wiki">def valuation(m, *args1, **args2):
return m.valuation(*args1, **args2)
</pre><p>
or
</p>
<pre class="wiki">def valuation(m, *args1, **args2):
if m == 0:
return sage.rings.all.infinity
return m.valuation(*args1, **args2)
</pre><p>
I haven't tried and/or ran any doctests with this, but anything that is broken by this isn't good coding practice, as you said :) Plus this fixes my example with <code>valuation(x^2)</code>.
</p>
TicketmstrengMon, 02 Aug 2010 14:46:15 GMTcc set
https://trac.sagemath.org/ticket/9652#comment:3
https://trac.sagemath.org/ticket/9652#comment:3
<ul>
<li><strong>cc</strong>
<em>mstreng</em> added
</li>
</ul>
TicketmderickxMon, 02 Aug 2010 15:06:53 GMT
https://trac.sagemath.org/ticket/9652#comment:4
https://trac.sagemath.org/ticket/9652#comment:4
<p>
I guess the code underneath there is for python integers, but I'm not sure. I will look into your questions when I have the time.
</p>
<p>
ps. similar practices seem to be going on a lot in arith.py I created a discussion on sage-devel. This discussion is not so much to discuss this specific example in detail, but to find a nice general solution for dealing with the problems involved when both using global functions an class methods to do the same thing.
</p>
<p>
I guess your remark about just passing all the aruments to the class method instead of having a static signature should be of value in that discussion also.
</p>
TicketmderickxMon, 02 Aug 2010 16:57:26 GMT
https://trac.sagemath.org/ticket/9652#comment:5
https://trac.sagemath.org/ticket/9652#comment:5
<p>
The code is indeed for python integers, and produces the right output in that case as the following shows.
</p>
<pre class="wiki">sage: a=344r
sage: type(a)
<type 'int'>
sage: hasattr(a,"valuation")
False
sage: valuation(a,2)
3
</pre><p>
I agree with you that that code should not be excecuted on general ring elements.
</p>
<p>
It crashes on local elements of the symbolic ring for example.
</p>
<pre class="wiki">sage: var("z")
z
sage: valuation(z^2,z)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
/Applications/sage/devel/sage-mderickx/<ipython console> in <module>()
/Applications/sage/local/lib/python2.6/site-packages/sage/rings/arith.pyc in valuation(m, p)
604 r = 0
605 power = p
--> 606 while not (m % power): # m % power == 0
607 r += 1
608 power *= p
TypeError: unsupported operand type(s) for %: 'sage.symbolic.expression.Expression' and 'sage.symbolic.expression.Expression'
</pre><p>
.
</p>
<p>
In your remark 2. The code could also crash, for example try (1/2) % 2.
I tried to find some real examples in sage, but I can't seem to find how to localize rings in sage. Maybe it's not implemented yet. The only thing I could find is: <a href="http://www.sagemath.org/doc/reference/coercion.html#example">http://www.sagemath.org/doc/reference/coercion.html#example</a>.
</p>
<p>
My failing quest to find other examples than python integers lead me to believe that this code should indeed only be excecuted on python integers.
</p>
TicketmstrengMon, 02 Aug 2010 17:18:04 GMT
https://trac.sagemath.org/ticket/9652#comment:6
https://trac.sagemath.org/ticket/9652#comment:6
<p>
Ah yes, python integers. Then perhaps first test if m is a python integer and call <code>Integer(m).valuation(*args1, **args2)</code> if it is and <code>m.valuation(*args1, **args2)</code> otherwise. I think you should leave out the <code>m == 0</code> check because nobody wants <code>valuation(0, 0)</code> or <code>valuation(0, 'whatever')</code> to give any output.
</p>
TicketmstrengMon, 02 Aug 2010 18:06:21 GMT
https://trac.sagemath.org/ticket/9652#comment:7
https://trac.sagemath.org/ticket/9652#comment:7
<p>
<code>valuation(int(1), int(1))</code> goes into an infinite loop
</p>
TicketmderickxMon, 02 Aug 2010 18:38:02 GMT
https://trac.sagemath.org/ticket/9652#comment:8
https://trac.sagemath.org/ticket/9652#comment:8
<p>
Ok so the code behaves buggy for valuation(1r,1r). I really think we should just let sage integers handle it. I really don't like having unneccery double code. Especially if one of the two is buggy.
</p>
<p>
By the way, the problem with valuation(0r,0r) is not just a problem of this code, it also happes with valuation on sage ints. Should we report that as a bug?
</p>
<pre class="wiki">sage: 0.valuation(0)
+Infinity
sage: 8.valuation(0)
ValueError Traceback (most recent call last)
...
ValueError: You can only compute the valuation with respect to a integer larger than 1.
</pre><p>
ps. if you want a number to not be converted to a sage integer by the preparser you should just put an r behind the number. Or else the preparser will generate the following silly code:
</p>
<pre class="wiki">sage: preparse("valuation(int(1),int(1)")
'valuation(int(Integer(1)),int(Integer(1))'
</pre><p>
What you want is this:
</p>
<pre class="wiki">sage: preparse("valuation(1r,1r)")
'valuation(1,1)'
</pre>
TicketmderickxMon, 02 Aug 2010 18:57:05 GMTattachment set
https://trac.sagemath.org/ticket/9652
https://trac.sagemath.org/ticket/9652
<ul>
<li><strong>attachment</strong>
set to <em>smallfix1-arith_valuation.patch</em>
</li>
</ul>
TicketcremonaSun, 22 Aug 2010 13:16:42 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:9
https://trac.sagemath.org/ticket/9652#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
You have added the extra arguments but have not documented them, or given any examples where they might be used. That's not good!
</p>
<p>
For the main code, why not do
</p>
<pre class="wiki">try:
return m.valuation(p)
except AttributeError:
pass
try:
return Integer(m).valuation(Integer(p))
except (something):
raise an error
</pre>
TicketmstrengSun, 22 Aug 2010 19:55:10 GMT
https://trac.sagemath.org/ticket/9652#comment:10
https://trac.sagemath.org/ticket/9652#comment:10
<p>
Wasn't the whole point of the extra arguments *args1, <strong>args2 to make the global function <code>valuation</code> also work for power series? It doesn't in the current Sage, and it wouldn't with John's suggestion, but actually, it also doesn't with Maarten's patch.
</strong></p>
<p>
To make it work, remove p. That is, replace "<code>p,*args1, **args2</code>" by "<code>*args1, **args2</code>" or simply "<code>*args1</code>" both times it occurs in your patch.
</p>
<p>
In the EXAMPLES block, you could add
</p>
<pre class="wiki">sage: y = QQ['y'].gen()
sage: valuation(y^3, y)
3
sage: x = QQ[['x']].gen()
sage: valuation(x^2)
2
</pre><p>
This doesn't really belong to the topic of this patch, but as you are rewriting the function anyway...
</p>
TicketcremonaSun, 22 Aug 2010 20:57:58 GMT
https://trac.sagemath.org/ticket/9652#comment:11
https://trac.sagemath.org/ticket/9652#comment:11
<p>
That looks better!
</p>
TicketmderickxMon, 06 Sep 2010 17:21:54 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:12
https://trac.sagemath.org/ticket/9652#comment:12
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I added a second patch which takes most of the comments into account. (please only apply the second patch, it got added as a second patch since I forgot to check the "replace existing file" box).
</p>
<p>
I didn't do anything with the "For the main code, why not do:" suggestion since to me it seemed to do the same thing but then with a different coding style. (My version is the "look before you leap version", and yours the "Easier to ask for forgiveness than permission."). If there are reasons to chose one above the other I would certainly do that. I know that my code will raise an attribute error sometimes, an error that you would rewrite in your case. But I guess that error should be not confusing at all to the enduser.
</p>
<pre class="wiki">
sage: valuation(graphs.!PetersenGraph())
---------------------------------------------------------------------------
!AttributeError Traceback (most recent call last)
/Users/maarten/<ipython console> in <module>()
/Applications/sage/local/lib/python2.6/site-packages/sage/rings/arith.pyc in valuation(m, *args1, **args2)
600 if isinstance(m,(int,long)):
601 m=Integer(m)
--> 602 return m.valuation(*args1, **args2)
603
604 def prime_powers(start, stop=None):
!AttributeError: 'Graph' object has no attribute 'valuation'
sage:
</pre>
TicketlftaberaWed, 08 Sep 2010 13:35:21 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:13
https://trac.sagemath.org/ticket/9652#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
You have to import "Integer" somewhere in the code:
</p>
<pre class="wiki">sage: a=int(3)
sage: valuation(a,2)
...
--> 695 m=Integer(m)
...
NameError: global name 'Integer' is not defined
</pre><p>
Add a doctest that uses an 'int' as argument to check that the previous code will work.
</p>
<p>
In the documentation, it says that valuation os zero is +Infinity but valuation with respect to zero is an error. However:
</p>
<pre class="wiki">sage: valuation(0,0)
+Infinity
sage: K=QQ['x']
sage: x=K.gen()
sage: valuation(x,K(0))
...
PariError
sage: valuation(K(0),K(0))
+Infinity
</pre>
TicketmstrengWed, 08 Sep 2010 14:17:14 GMT
https://trac.sagemath.org/ticket/9652#comment:14
https://trac.sagemath.org/ticket/9652#comment:14
<p>
Why do you let the code work for float, but not for <a class="missing wiki">RealNumber?</a>? (I wouldn't let it work for float at all.)
</p>
TicketlftaberaWed, 08 Sep 2010 15:48:07 GMT
https://trac.sagemath.org/ticket/9652#comment:15
https://trac.sagemath.org/ticket/9652#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9652#comment:14" title="Comment 14">mstreng</a>:
</p>
<blockquote class="citation">
<p>
Why do you let the code work for float, but not for <a class="missing wiki">RealNumber?</a>? (I wouldn't let it work for float at all.)
</p>
</blockquote>
<p>
Could you elaborate that? For me, the code fails for floats with an <a class="missing wiki">AttributeError?</a> exception (as well as <a class="missing wiki">RealNumber?</a>).
</p>
TicketmderickxWed, 08 Sep 2010 18:27:57 GMT
https://trac.sagemath.org/ticket/9652#comment:16
https://trac.sagemath.org/ticket/9652#comment:16
<p>
I talked with Marco (mstreng) in real live. And his comment should be ignored as it was based on something he misread.
</p>
TicketmderickxWed, 08 Sep 2010 21:36:19 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:17
https://trac.sagemath.org/ticket/9652#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I added the import and doctest as lftabera suggested.
</p>
<p>
I didn't do anything with the comment about QQ<a class="missing wiki">x?</a> since I already changed the documentation of the global function to say that you really should look at the documentation of the attribute functions. And the behaviour you show is consistent with the documentation of the attribute function.
</p>
TicketlftaberaFri, 10 Sep 2010 07:45:39 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:18
https://trac.sagemath.org/ticket/9652#comment:18
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I found the following doctest failures (4.5.2):
</p>
<p>
sage -t -long "devel/sage-devel/sage/quadratic_forms/count_local_2.pyx"<br />
sage -t -long "devel/sage-devel/sage/quadratic_forms/quadratic_form<span class="underline">count_local_2.py"<br />
sage -t -long "devel/sage-devel/sage/quadratic_forms/quadratic_form</span>local_density_congruence.py"<br />
sage -t -long "devel/sage-devel/sage/quadratic_forms/quadratic_form<span class="underline">local_representation_conditions.py"<br />
</span></p>
<p>
all related with
</p>
<p>
<a class="missing wiki">AttributeError?</a>: 'sage.rings.finite_rings.integer_mod.IntegerMod_int' object has no attribute 'valuation'
</p>
<p>
Also, in the code, you import Integers in the case of 'int' or 'float'. However, the local namespace of arith contains ZZ. I have checked and it is fastr to use ZZ(m) instead of importing Integer and using Integer(m). This is a corner case though.
</p>
TicketmderickxFri, 10 Sep 2010 12:18:22 GMT
https://trac.sagemath.org/ticket/9652#comment:19
https://trac.sagemath.org/ticket/9652#comment:19
<p>
Is there somewhere in the math literature a definition of what a valuation should be for "Ring of integers modulo n", cause the way it behaves right now in sage doesn't agree with my definition of what conditions a valuation should satisfy. If v is a valuation then in particular I would like to have v(a*b)=v(a)+v(b). But in sage right now you get the following:
</p>
<blockquote>
<p>
sage: for i in R:
</p>
</blockquote>
<p>
....: for j in R:
....: if not valuation(i,Integer(3))+valuation(j,Integer(3))==valuation(i*j,Integer(3)):
....: print i,j
....:
3 3
3 6
6 3
6 6
</p>
TicketmderickxFri, 10 Sep 2010 12:19:03 GMT
https://trac.sagemath.org/ticket/9652#comment:20
https://trac.sagemath.org/ticket/9652#comment:20
<p>
oops, now with the right formatting:
</p>
<pre class="wiki">
sage: for i in R:
....: for j in R:
....: if not valuation(i,Integer(3))+valuation(j,Integer(3))==valuation(i*j,Integer(3)):
....: print i,j
....:
3 3
3 6
6 3
6 6
</pre>
TicketmderickxFri, 10 Sep 2010 12:19:59 GMT
https://trac.sagemath.org/ticket/9652#comment:21
https://trac.sagemath.org/ticket/9652#comment:21
<p>
By the way: R=ZZ.quo(9)
</p>
TicketlftaberaFri, 10 Sep 2010 13:12:58 GMT
https://trac.sagemath.org/ticket/9652#comment:22
https://trac.sagemath.org/ticket/9652#comment:22
<p>
I am afraid that there are quite a bit of algorithms in SAGE that deal with <a class="missing wiki">IntegerModInt?</a> as plain Integer. The algorithms work, but you may fail to give a standar mathematical sense to them. This would be an example.
</p>
TicketmderickxFri, 10 Sep 2010 13:39:33 GMT
https://trac.sagemath.org/ticket/9652#comment:23
https://trac.sagemath.org/ticket/9652#comment:23
<p>
Ok, then i'll just move the relevant parts of the old code so it becomes an attribute function of <a class="missing wiki">IntegerModInt?</a> together with a note section that sais that it's not a valuation in the mathematical sense.
</p>
TicketmderickxSat, 11 Sep 2010 08:53:24 GMTowner changed
https://trac.sagemath.org/ticket/9652#comment:24
https://trac.sagemath.org/ticket/9652#comment:24
<ul>
<li><strong>owner</strong>
changed from <em>AlexGhitza</em> to <em>mderickx</em>
</li>
</ul>
TicketmderickxSat, 11 Sep 2010 08:54:12 GMTattachment set
https://trac.sagemath.org/ticket/9652
https://trac.sagemath.org/ticket/9652
<ul>
<li><strong>attachment</strong>
set to <em>smallfix1-arith_valuation.2.patch</em>
</li>
</ul>
<p>
Use this one, the other is an old version
</p>
TicketmderickxSat, 11 Sep 2010 08:56:48 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:25
https://trac.sagemath.org/ticket/9652#comment:25
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Ok, I moved the code, and the integer mod classes now also have a valuation attribute. All doctest should pass now when patching against 4.4.4 (sorry haven't updated in a while).
</p>
TicketmderickxSun, 12 Sep 2010 10:52:26 GMTattachment set
https://trac.sagemath.org/ticket/9652
https://trac.sagemath.org/ticket/9652
<ul>
<li><strong>attachment</strong>
set to <em>smallfix1-arith_valuation.2.2.patch</em>
</li>
</ul>
<p>
Use this one, the other is an old version
</p>
TicketlftaberaTue, 14 Sep 2010 11:26:54 GMTattachment set
https://trac.sagemath.org/ticket/9652
https://trac.sagemath.org/ticket/9652
<ul>
<li><strong>attachment</strong>
set to <em>smallfix1-arith_valuation.2.3.patch</em>
</li>
</ul>
<p>
previous patch with clenaer comment in the header
</p>
TicketlftaberaTue, 14 Sep 2010 11:27:08 GMTattachment set
https://trac.sagemath.org/ticket/9652
https://trac.sagemath.org/ticket/9652
<ul>
<li><strong>attachment</strong>
set to <em>smallfix1-arith_valuation-doctest.2.3.patch</em>
</li>
</ul>
<p>
additional doctest
</p>
TicketlftaberaTue, 14 Sep 2010 11:30:37 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:26
https://trac.sagemath.org/ticket/9652#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
Everything looks fine. Applies and passes doctest in 4.5.3
</p>
<p>
I have modified the header of the patch in smallfix1-arith_valuation.2.3.patch and I have added another patch with a new doctest regarding a previous issue that arrived before valuation(1r,1r) not terminating.
</p>
<p>
Since I have not touched the code, I fell I can give possitive review, but before doing so, mderickx, give me the OK for the change in the header of your patch.
</p>
TicketmderickxTue, 14 Sep 2010 11:59:29 GMTcc changed
https://trac.sagemath.org/ticket/9652#comment:27
https://trac.sagemath.org/ticket/9652#comment:27
<ul>
<li><strong>cc</strong>
<em>lftabera</em> added
</li>
</ul>
<p>
I'm ok with the changes. Hooray, my first patch got through :).
</p>
TicketlftaberaTue, 14 Sep 2010 12:32:29 GMTstatus, description changed
https://trac.sagemath.org/ticket/9652#comment:28
https://trac.sagemath.org/ticket/9652#comment:28
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9652?action=diff&version=28">diff</a>)
</li>
</ul>
TicketlftaberaTue, 14 Sep 2010 12:32:39 GMTstatus changed
https://trac.sagemath.org/ticket/9652#comment:29
https://trac.sagemath.org/ticket/9652#comment:29
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketmpatelWed, 15 Sep 2010 11:06:49 GMTreviewer, milestone, author set
https://trac.sagemath.org/ticket/9652#comment:30
https://trac.sagemath.org/ticket/9652#comment:30
<ul>
<li><strong>reviewer</strong>
set to <em>Marco Streng, John Cremona, Luis Felipe Tabera</em>
</li>
<li><strong>milestone</strong>
set to <em>sage-4.6</em>
</li>
<li><strong>author</strong>
set to <em>Maarten Derickx</em>
</li>
</ul>
<p>
Please remember to update the "Author(s)" and "Reviewer(s)" fields.
</p>
TicketmpatelWed, 15 Sep 2010 11:14:02 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/9652#comment:31
https://trac.sagemath.org/ticket/9652#comment:31
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.6.alpha1</em>
</li>
</ul>
Ticket