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.

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

Closed
wants to merge 1 commit into from
Closed

Conversation

eschnett
Copy link
Contributor

@eschnett eschnett commented May 7, 2020

Closes #33341.

base/number.jl Outdated Show resolved Hide resolved
base/complex.jl Outdated Show resolved Hide resolved
@StefanKarpinski
Copy link
Sponsor Member

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)

@ararslan ararslan added maths Mathematical functions needs news A NEWS entry is required for this change labels May 7, 2020
@eschnett
Copy link
Contributor Author

eschnett commented May 8, 2020

@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 cispi(1) == cispi(1.0)...

@simeonschaub
Copy link
Member

simeonschaub commented May 8, 2020

Great to have this, I have needed that before! I am wondering, whether it would make sense to have an optimized sincospi here. Looking at the implementations of sinpi and sincos, it looks like there are some optimization opportunities by calculating them together. Probably a separate issue though.

@antoine-levitt
Copy link
Contributor

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.

@simeonschaub simeonschaub mentioned this pull request May 8, 2020
@simeonschaub
Copy link
Member

IIUC the advantage of sinpi and friends is not better performance, but better accuracy and exact results for multiples of 1/2.

base/number.jl Outdated
Comment on lines 95 to 96
`signbit` is the inverse of `cispi` (which calculates `(-1)^n`):
`signbit(cispi(b)) == b`. Also, `cispi(n) * abs(n) == n`.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 Bools 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?

Copy link
Contributor Author

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.

@eschnett
Copy link
Contributor Author

eschnett commented May 8, 2020

@simeonschaub Yes, there should be an optimized sincospi as well. This should be even faster than sincos since one doesn't have to divide by pi.

@simeonschaub simeonschaub mentioned this pull request May 9, 2020
@StefanKarpinski
Copy link
Sponsor Member

There appears to have been a git merge snafu here.

@eschnett
Copy link
Contributor Author

@StefanKarpinski snafu corrected.

@ViralBShah
Copy link
Member

Bump. Is it good to merge? Still needs NEWS.

@eschnett
Copy link
Contributor Author

I thought the consensus was to not have cospi / sinpi with special cases for integer arguments? If so, this PR should be rejected in favour of a new function (e.g. bitsign) that is intended as inverse to signbit, without direct connotation to trigonometry.

@simonbyrne
Copy link
Contributor

sincospi is now merged (#35816)

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

We can now take advantage of #35816. What cispi returns for integers, should probably follow sincospi for now, until #35820 reaches a consensus.

Comment on lines +544 to +546
cispi(sign::Bool) = sign ? -1 : 1
cispi(sign::Integer) = oftype(sign, cispi(isodd(sign)))
cispi(theta::Real) = Complex(cospi(theta), sinpi(theta))
Copy link
Member

@simeonschaub simeonschaub Jun 23, 2020

Choose a reason for hiding this comment

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

Suggested change
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))...)

Comment on lines +574 to +576
function cispi(z::Complex)
cospi(z) + im*sinpi(z)
end
Copy link
Member

@simeonschaub simeonschaub Jun 23, 2020

Choose a reason for hiding this comment

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

Suggested change
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.

@simeonschaub
Copy link
Member

What's the current status of this? Would definitely be nice to have for consistency, now that we have sincospi.

@simeonschaub
Copy link
Member

Bump. @eschnett Do you still intend to finish this? I do think this has value outside being the inverse of signbit.

@eschnett
Copy link
Contributor Author

@simeonschaub No, I don't think any more that this is a good idea. cispi for floating point numbers makes sense, but not for integers.

@simeonschaub
Copy link
Member

I am fine with not special-casing Integer, I just want the generic definitions for real and complex numbers.

@jmert
Copy link
Contributor

jmert commented Nov 13, 2020

Is there any chance this can get finished and merged (just the cispi part, not the relation to signbit part)?

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

jmert pushed a commit to jmert/julia that referenced this pull request Nov 15, 2020
Keno pushed a commit that referenced this pull request Nov 29, 2020
* Implement cispi

* Review comments in #35792

* Remove docs relating cispi and signbit

* Review comment, more expressive function doc.

Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>

* Switch examples to show complex outputs

Co-authored-by: Erik Schnetter <schnetter@gmail.com>
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
@Keno Keno closed this Nov 29, 2020
@eschnett eschnett deleted the eschnett/cispi branch August 14, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add function bitsign (inverse of signbit)
10 participants