Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add "sign" method #3630

Open
wants to merge 9 commits into
base: development
Choose a base branch
from
Open

Add "sign" method #3630

wants to merge 9 commits into from

Conversation

d-torrance
Copy link
Member

I'm amazed that we don't have this already!

We add a sign method that wraps the corresponding functions in GMP and MPFR (and returns $\frac{z}{|z|}$ for nonzero complex $z$). For example:

i1 : sign 5

o1 = 1

i2 : sign 0

o2 = 0

i3 : sign(-3)

o3 = -1

@d-torrance d-torrance requested a review from mahrud January 19, 2025 21:54
@d-torrance d-torrance force-pushed the sign branch 5 times, most recently from f5087a0 to 5b59ebe Compare January 20, 2025 15:24
@d-torrance d-torrance mentioned this pull request Jan 20, 2025
Draft
@d-torrance
Rename "sign" -> "signbit" in d directory
7185f30 
More descriptive since it gives the value of the sign *bit* and wraps
mpfr_signbit.  Clears the way for us to use "sign" for wrapping
various *_sgn functions from GMP and MPFR.

Also simplify remove unused sign(RRi).
@d-torrance
Add sign(ZZ) to d directory (wraps mpz_sgn)
d36f073
@d-torrance
Add sign(RR) to d directory (wraps mpfr_sgn)
a92e80b
@d-torrance
Add sign(QQ) to d directory (wraps mpq_sgn)
26e5aec
@d-torrance
Make interpreter "sign" functions available at top level
a93287c
@d-torrance
Add top-level "sign" method
0a3fff3
@d-torrance
Remove conflicts in packages now that "sign" is in Core
2fe9d89 
Either remove now-redundant methods or rename "sign" variables to "sgn".
@d-torrance
Document "sign"
6042140
@d-torrance
Add tests for "sign"
326c308
@mahrud
Copy link
Member

mahrud commented Jan 20, 2025

I'll be honest, I don't think I like sign being reserved for something that in principle is as simple as x // abs x. Sign of a permutation is probably useful as a method, but again the question is whether it's worth reserving a symbol which may be used in people's code for other things.

@d-torrance
Copy link
Member Author

We already have two packages (GradedLieAlgebras and Permutations) there were exporting their own sign methods, so I think it makes sense to move the actual method function definition to Core. There were also a couple other packages that were defining sign internally with the same behavior that I'm proposing, and this simplifies things by moving all that code to one place.

There were a number of packages that used sign as a variable. I went ahead and changed those to sgn. There's certainly a chance this might break other code out in the wild, but that's a risk any time we add something new.

mahrud
mahrud requested changes Jan 22, 2025
View reviewed changes
Copy link
Member

@mahrud mahrud left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not a fan of the places where a variable or function named sign is changed to sgn. Why not define sgn instead? Also matches the notation here.

M2/Macaulay2/packages/Macaulay2Doc/functions/sign-doc.m2
@@ -0,0 +1,26 @@
doc ///
Key
sign
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a shared symbol, then it should be documented similar to other shared symbols, with links to nodes in other packages that enhance it.

M2/Macaulay2/packages/GradedLieAlgebras.m2
@@ -72,7 +72,6 @@ export {
"mbRing",
"minimalModel",
"normalForm",
"sign",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed it from here, but several places in the documentation link to sign, which now goes to an irrelevant node in Macaulay2Doc.

M2/Macaulay2/packages/Permutations.m2
@@ -33,7 +33,6 @@ export {
"isCDG",
"foataBijection",
"ord",
"sign",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem here, this change mangles the documentation links to sign.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mahrud mahrud mahrud requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@d-torrance @mahrud