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

StackOverflowError when computing sincospi(2±1) #513

Closed
kvnoct opened this issue Mar 29, 2022 · 5 comments · Fixed by #567
Closed

StackOverflowError when computing sincospi(2±1) #513

kvnoct opened this issue Mar 29, 2022 · 5 comments · Fixed by #567

Comments

@kvnoct
Copy link

kvnoct commented Mar 29, 2022

I got StackOverflowError when computing sincospi(2±1). Here's the code to reproduce

using IntervalArithmetic
x = 1..3
sincospi(x)
@dpsanders
Copy link
Member

I don't think we implemented sincospi, so it's probably falling back to the generic version. (I didn't even know there was such a function.)

Try splitting that up into two pieces.

@lbenet
Copy link
Member

lbenet commented Mar 29, 2022

Thanks for reporting @kvnoct!

Both, sincos and sincospi are computed using methods defined in Base:

julia> @which sincos(x)
sincos(x) in Base.Math at special/trig.jl:205

julia> sincos(x)  # OK
([0.14112, 1], [-0.989993, 0.540303])

julia> @which sincospi(x)
sincospi(x::Real) in Base.Math at special/trig.jl:940

julia> sincospi(x)
ERROR: StackOverflowError:
Stacktrace:
 [1] sincospi(x::Interval{Float64}) (repeats 79984 times)
   @ Base.Math ./special/trig.jl:940

The method used for sincos(x) finally corresponds to (sin(x), cos(x)), and thus is ok. For sincospi, this line calls itself again and again, which yields the StackOverflow. As @dpsanders suggests, you can solve it by "splitting that up into two pieces":

julia> import Base: sincospi

julia> sincospi(x::Interval) = (sinpi(x), cospi(x))
sincospi (generic function with 7 methods)

julia> sincospi(x)
([-1, 1], [-1, 1])

@kvnoct
Copy link
Author

kvnoct commented Mar 30, 2022

Thanks @lbenet @dpsanders!

Actually I was using cispi, which calls the sincospi

cispi(theta::Real) = Complex(reverse(sincospi(theta))...)

I guess I will define a workaround to handle the intervals in the meantime

@dpsanders
Copy link
Member

Note that there is only partial support for complex intervals so far.

@lucaferranti
Copy link
Member

It might be nice to define sincospi(x::Interval) so that it exploits "shared parts" of sin and cos computation and computes both at once faster than computing both separately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants