Sage: Ticket #7828: There should be a top-level sign() function.
https://trac.sagemath.org/ticket/7828
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/7828
Trac 1.1.6robertwbSun, 03 Jan 2010 06:05:35 GMT
https://trac.sagemath.org/ticket/7828#comment:1
https://trac.sagemath.org/ticket/7828#comment:1
<p>
Apparently, it's called <code>sgn</code>, but perhaps we should have sign as an alias.
</p>
TicketmhansenSun, 03 Jan 2010 18:04:32 GMT
https://trac.sagemath.org/ticket/7828#comment:2
https://trac.sagemath.org/ticket/7828#comment:2
<p>
Especially, if some of the methods are .sign().
</p>
TicketkcrismanWed, 26 May 2010 19:17:47 GMTauthor set
https://trac.sagemath.org/ticket/7828#comment:3
https://trac.sagemath.org/ticket/7828#comment:3
<ul>
<li><strong>author</strong>
set to <em>Karl-Dieter Crisman</em>
</li>
</ul>
<p>
Okay, this makes lots of sense, and in fact we should check hasattr with that first. Patch coming up, which should work but will also allow (perhaps this is not good):
</p>
<pre class="wiki"> sage: p = PermutationGroupElement('(3,4,8,7,9)')
sage: p.sign()
1
sage: sign(p)
1
</pre>
TicketkcrismanWed, 26 May 2010 19:26:47 GMTattachment set
https://trac.sagemath.org/ticket/7828
https://trac.sagemath.org/ticket/7828
<ul>
<li><strong>attachment</strong>
set to <em>trac_7828-sign.patch</em>
</li>
</ul>
<p>
Based on 4.4.2
</p>
TicketkcrismanWed, 26 May 2010 19:27:08 GMTstatus changed
https://trac.sagemath.org/ticket/7828#comment:4
https://trac.sagemath.org/ticket/7828#comment:4
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketcremonaThu, 27 May 2010 21:11:30 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/7828#comment:5
https://trac.sagemath.org/ticket/7828#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>John Cremona</em>
</li>
</ul>
<p>
Looks good, applies fine to 4.4.3.alpha0 and tests pass.
</p>
<p>
I did wonder whether it would be better to return a Sage integer rather than an int?
</p>
<p>
Also, I looked for places where sgn() was used/defined and found a redundant definition of sgn() in quadratic_forms/extras.py, which I am removing in another ticket (<a class="closed ticket" href="https://trac.sagemath.org/ticket/9068" title="defect: remove redundant sgn function from quadratic_forms/extras (closed: fixed)">#9068</a>).
</p>
TicketkcrismanFri, 28 May 2010 00:10:31 GMT
https://trac.sagemath.org/ticket/7828#comment:6
https://trac.sagemath.org/ticket/7828#comment:6
<blockquote class="citation">
<p>
I did wonder whether it would be better to return a Sage integer rather than an int?
</p>
</blockquote>
<p>
Hmm, that is an interesting thing I should have considered but did not. As long as we are consistent, that's probably the main thing, though it is often helpful to return something that has the Integer methods... Are there any current sign()/sgn() methods that return something other than an int?
</p>
<p>
Usually one just adds or multiplies it with Integers, but I could imagine that sometimes the output itself would be important and that it should also then be an Integer. If so... another ticket, or on this one?
</p>
TicketcremonaFri, 28 May 2010 08:18:33 GMTstatus changed
https://trac.sagemath.org/ticket/7828#comment:7
https://trac.sagemath.org/ticket/7828#comment:7
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Well, I did look for other places where methods sgn() or sign() were defined; since in fact I have another comment, which is that as well as looking to see if x has a method sign() you should also look for a method sgn(). The only thing I found was that function in quadratic_forms, and that distracted me from making this comment.
</p>
<p>
I will do the following now, and report back:
</p>
<ol><li>Apply both your patch and mine at <a class="closed ticket" href="https://trac.sagemath.org/ticket/9068" title="defect: remove redundant sgn function from quadratic_forms/extras (closed: fixed)">#9068</a>
</li><li>Change the function you changed in two ways: making the return type Integer and also checking for x.sgn()
</li><li>Test the whole library.
</li></ol><p>
For the moment I have reverted this to "needs work".
</p>
TicketcremonaFri, 28 May 2010 09:02:54 GMTattachment set
https://trac.sagemath.org/ticket/7828
https://trac.sagemath.org/ticket/7828
<ul>
<li><strong>attachment</strong>
set to <em>trac_7828-reviewer.patch</em>
</li>
</ul>
<p>
Apply after previous
</p>
TicketcremonaFri, 28 May 2010 09:05:20 GMTstatus, author, component, milestone changed; keywords set
https://trac.sagemath.org/ticket/7828#comment:8
https://trac.sagemath.org/ticket/7828#comment:8
<ul>
<li><strong>keywords</strong>
<em>sign</em> <em>sgn</em> added
</li>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
changed from <em>Karl-Dieter Crisman</em> to <em>Karl-Dieter Crisman, John Cremona</em>
</li>
<li><strong>component</strong>
changed from <em>algebra</em> to <em>basic arithmetic</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-wishlist</em> to <em>sage-4.4.3</em>
</li>
</ul>
<p>
OK, I did that (see the reviewer patch). All tests pass (note that I also had my patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/9068" title="defect: remove redundant sgn function from quadratic_forms/extras (closed: fixed)">#9068</a> applied).
</p>
<p>
I think we need an independent reviewer of the combined changes (preferably of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9068" title="defect: remove redundant sgn function from quadratic_forms/extras (closed: fixed)">#9068</a> also) so I have put it back to "needs review".
</p>
TicketrobertwbFri, 28 May 2010 17:24:42 GMTstatus changed
https://trac.sagemath.org/ticket/7828#comment:9
https://trac.sagemath.org/ticket/7828#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Looks good to me.
</p>
TicketAlexGhitzaSat, 05 Jun 2010 00:57:23 GMTreviewer changed
https://trac.sagemath.org/ticket/7828#comment:10
https://trac.sagemath.org/ticket/7828#comment:10
<ul>
<li><strong>reviewer</strong>
changed from <em>John Cremona</em> to <em>John Cremona, Robert Bradshaw</em>
</li>
</ul>
TicketmhansenSun, 06 Jun 2010 08:37:15 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/7828#comment:11
https://trac.sagemath.org/ticket/7828#comment:11
<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.4.4.alpha0</em>
</li>
</ul>
TicketburcinThu, 09 Sep 2010 09:32:29 GMT
https://trac.sagemath.org/ticket/7828#comment:12
https://trac.sagemath.org/ticket/7828#comment:12
<p>
Was there a concious decision in this ticket (or elsewhere) not to standardize on either <code>sign()</code> or <code>sgn()</code>. I just saw the relevant part of <code>sage/functions/generalized.py</code>, and thought one of these is redundant.
</p>
TicketkcrismanThu, 09 Sep 2010 13:11:06 GMT
https://trac.sagemath.org/ticket/7828#comment:13
https://trac.sagemath.org/ticket/7828#comment:13
<p>
I think the point was that not everyone would think of <code>sign()</code> or <code>sgn()</code> automatically; depending on where you come from mathematically, one or the other is more natural. This doesn't seem to me to be a problem; we have lots of aliases, and it seems very unlikely that there would be confusion once someone saw both of them, as sgn is clearly short for sign.
</p>
<p>
Or maybe you mean we should pick one and leave the other one as an unspoken alias.
</p>
<p>
However, I guess in this ticket and <a class="closed ticket" href="https://trac.sagemath.org/ticket/9068" title="defect: remove redundant sgn function from quadratic_forms/extras (closed: fixed)">#9068</a> there is an implicit assumption that the methods (as opposed to functions) should be called <code>.sign()</code>. Is that bad?
</p>
TicketburcinMon, 13 Sep 2010 09:08:35 GMT
https://trac.sagemath.org/ticket/7828#comment:14
https://trac.sagemath.org/ticket/7828#comment:14
<p>
I suggest we choose <code>sign()</code> as the convention and make <code>sgn()</code> an alias where necessary. Then we don't need to check for the existence of both <code>.sign()</code> and <code>.sgn()</code> methods. That code (around line 474 of <code>sage/functions/generalized.py</code>) suggests we encourage sloppy programming.
</p>
<p>
Shall I open a ticket to look through the library for <code>sgn()</code> and <code>sign()</code> functions and change them appropriately?
</p>
TicketkcrismanMon, 13 Sep 2010 14:52:44 GMT
https://trac.sagemath.org/ticket/7828#comment:15
https://trac.sagemath.org/ticket/7828#comment:15
<p>
I think that cremona already did this, but put this in there just in case there was another one. So are you suggesting that the reviewer patch should be modified? I think that the fear is that someone will put in a <code>.sgn()</code> method and won't realize it won't work; on the other hand, one could check for <code>.sgn()</code> and raise an error, but that also would make it look weird. Though I wouldn't say sloppy, but rather decentralized programming.
</p>
Ticket