Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#7828 closed defect (fixed)

There should be a top-level sign() function.

Reported by: Robert Bradshaw Owned by: Alex Ghitza
Priority: minor Milestone: sage-4.4.4
Component: basic arithmetic Keywords: sign sgn
Cc: Merged in: sage-4.4.4.alpha0
Authors: Karl-Dieter Crisman, John Cremona Reviewers: John Cremona, Robert Bradshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description


Attachments (2)

trac_7828-sign.patch (2.0 KB) - added by Karl-Dieter Crisman 13 years ago.
Based on 4.4.2
trac_7828-reviewer.patch (1.6 KB) - added by John Cremona 13 years ago.
Apply after previous

Download all attachments as: .zip

Change History (17)

comment:1 Changed 13 years ago by Robert Bradshaw

Apparently, it's called sgn, but perhaps we should have sign as an alias.

comment:2 Changed 13 years ago by Mike Hansen

Especially, if some of the methods are .sign().

comment:3 Changed 13 years ago by Karl-Dieter Crisman

Authors: Karl-Dieter Crisman

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):

            sage: p = PermutationGroupElement('(3,4,8,7,9)')
            sage: p.sign()
            1
            sage: sign(p)
            1

Changed 13 years ago by Karl-Dieter Crisman

Attachment: trac_7828-sign.patch added

Based on 4.4.2

comment:4 Changed 13 years ago by Karl-Dieter Crisman

Status: newneeds_review

comment:5 Changed 13 years ago by John Cremona

Reviewers: John Cremona
Status: needs_reviewpositive_review

Looks good, applies fine to 4.4.3.alpha0 and tests pass.

I did wonder whether it would be better to return a Sage integer rather than an int?

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 (#9068).

comment:6 in reply to:  5 Changed 13 years ago by Karl-Dieter Crisman

I did wonder whether it would be better to return a Sage integer rather than an int?

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?

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?

comment:7 Changed 13 years ago by John Cremona

Status: positive_reviewneeds_work

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.

I will do the following now, and report back:

  1. Apply both your patch and mine at #9068
  2. Change the function you changed in two ways: making the return type Integer and also checking for x.sgn()
  3. Test the whole library.

For the moment I have reverted this to "needs work".

Changed 13 years ago by John Cremona

Attachment: trac_7828-reviewer.patch added

Apply after previous

comment:8 Changed 13 years ago by John Cremona

Authors: Karl-Dieter CrismanKarl-Dieter Crisman, John Cremona
Component: algebrabasic arithmetic
Keywords: sign sgn added
Milestone: sage-wishlistsage-4.4.3
Status: needs_workneeds_review

OK, I did that (see the reviewer patch). All tests pass (note that I also had my patch from #9068 applied).

I think we need an independent reviewer of the combined changes (preferably of #9068 also) so I have put it back to "needs review".

comment:9 Changed 13 years ago by Robert Bradshaw

Status: needs_reviewpositive_review

Looks good to me.

comment:10 Changed 13 years ago by Alex Ghitza

Reviewers: John CremonaJohn Cremona, Robert Bradshaw

comment:11 Changed 13 years ago by Mike Hansen

Merged in: sage-4.4.4.alpha0
Resolution: fixed
Status: positive_reviewclosed

comment:12 Changed 12 years ago by Burcin Erocal

Was there a concious decision in this ticket (or elsewhere) not to standardize on either sign() or sgn(). I just saw the relevant part of sage/functions/generalized.py, and thought one of these is redundant.

comment:13 Changed 12 years ago by Karl-Dieter Crisman

I think the point was that not everyone would think of sign() or sgn() 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.

Or maybe you mean we should pick one and leave the other one as an unspoken alias.

However, I guess in this ticket and #9068 there is an implicit assumption that the methods (as opposed to functions) should be called .sign(). Is that bad?

comment:14 Changed 12 years ago by Burcin Erocal

I suggest we choose sign() as the convention and make sgn() an alias where necessary. Then we don't need to check for the existence of both .sign() and .sgn() methods. That code (around line 474 of sage/functions/generalized.py) suggests we encourage sloppy programming.

Shall I open a ticket to look through the library for sgn() and sign() functions and change them appropriately?

comment:15 Changed 12 years ago by Karl-Dieter Crisman

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 .sgn() method and won't realize it won't work; on the other hand, one could check for .sgn() and raise an error, but that also would make it look weird. Though I wouldn't say sloppy, but rather decentralized programming.

Note: See TracTickets for help on using tickets.