-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
New Julia function cispi #35792
New Julia function cispi #35792
Conversation
I like it! Nice simple function with very fast special cases for specific types. Feels like testing a few other values would be good, maybe: @test cispi(0.5) == 1
@test cispi(1.5) == -1
@test cispi(0.25) == Complex(sqrt(2)/2, sqrt(2)/2) |
@StefanKarpinski Those tests for fast special cases are a very good idea. I actually managed to have a bug in my implementation (now fixed). Now we even have |
Great to have this, I have needed that before! I am wondering, whether it would make sense to have an optimized |
For floats, is it faster (or at least as fast) as cis(pi*x)? Since cis calls an optimized sincos it could well be worse. |
IIUC the advantage of |
base/number.jl
Outdated
`signbit` is the inverse of `cispi` (which calculates `(-1)^n`): | ||
`signbit(cispi(b)) == b`. Also, `cispi(n) * abs(n) == n`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"signbit
is the inverse of cispi
"
Perhaps it would be more accurate with "a left inverse of cispi
when b
is a Bool
"
cispi(n) * abs(n) == n
, doesn't seem to hold for e.g., n=-2
I'm not sure how helpful this paragraph is for understanding the functionality of signbit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to use the terminology of "left inverse" since that's quite technical. I want to point to the fact that signbit
and cispi
are related. signbit
itself should be explained elsewhere – but a reminder here might be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling it inverse (or left inverse) without specifying that it is only valid for Bool
s seems like a quite puzzling statement. But I guess I mostly think that cispi
and signbit
are not that strongly related to motivate adding all this text to the succinct doc string of signbit
. If there really is a need to indicate a relation, perhaps a simple See also [`cispi`](@ref)
would be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the documentation. I now call them "related" functions.
@simeonschaub Yes, there should be an optimized |
There appears to have been a git merge snafu here. |
@StefanKarpinski snafu corrected. |
Bump. Is it good to merge? Still needs NEWS. |
I thought the consensus was to not have |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cispi(sign::Bool) = sign ? -1 : 1 | ||
cispi(sign::Integer) = oftype(sign, cispi(isodd(sign))) | ||
cispi(theta::Real) = Complex(cospi(theta), sinpi(theta)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cispi(sign::Bool) = sign ? -1 : 1 | |
cispi(sign::Integer) = oftype(sign, cispi(isodd(sign))) | |
cispi(theta::Real) = Complex(cospi(theta), sinpi(theta)) | |
cispi(theta::Real) = complex(reverse(sincospi(theta))...) |
function cispi(z::Complex) | ||
cospi(z) + im*sinpi(z) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function cispi(z::Complex) | |
cospi(z) + im*sinpi(z) | |
end | |
function cispi(z::Complex) | |
sipi, copi = sincospi(z) | |
return complex(real(copi) - imag(sipi), imag(copi) + real(sipi)) | |
end |
The second line might be a bit of a micro-optimization, but certainly won't hurt.
What's the current status of this? Would definitely be nice to have for consistency, now that we have |
Bump. @eschnett Do you still intend to finish this? I do think this has value outside being the inverse of |
@simeonschaub No, I don't think any more that this is a good idea. |
I am fine with not special-casing |
Is there any chance this can get finished and merged (just the (I can volunteer to cherry-pick onto a new branch/PR and deal with any further review if that'd help. It looks like the suggested changes above plus a few edits to doc strings is all that would be needed.) |
* Implement cispi * Review comments in #35792 * Remove docs relating cispi and signbit * Review comment, more expressive function doc. Co-authored-by: Simeon Schaub <[email protected]> * Switch examples to show complex outputs Co-authored-by: Erik Schnetter <[email protected]> Co-authored-by: Simeon Schaub <[email protected]>
Closes #33341.