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

Base.cis operating on angles in degrees first converts to radians #738

Closed
jecs opened this issue Aug 30, 2024 · 2 comments · Fixed by #745
Closed

Base.cis operating on angles in degrees first converts to radians #738

jecs opened this issue Aug 30, 2024 · 2 comments · Fixed by #745

Comments

@jecs
Copy link

jecs commented Aug 30, 2024

Base.cis, when called with an argument that is in degrees, first converts the argument to radians, and then calls sincos, resulting in output that is less precise. Possibly related to Issue 450.

This can be seen in the following snippet.

julia> cis(90u"°")
6.123233995736766e-17 + 1.0im

julia> @code_typed cis(90u"°")
CodeInfo(
1 ─ %1 = Base.getfield(x, :val)::Int64
│   %2 = Base.sitofp(Float64, %1)::Float64
│   %3 = Base.mul_float(%2, 0.017453292519943295)::Float64
│   %4 = invoke Base.sincos(%3::Float64)::Tuple{Float64, Float64}
│   %5 = Base.getfield(%4, 1)::Float64
│   %6 = Base.getfield(%4, 2)::Float64
│   %7 = %new(ComplexF64, %6, %5)::ComplexF64
└──      return %7
) => ComplexF64

However, when calling sincos directly, the correct method is chosen:

julia> sincos(90u"°")
(1.0, 0.0)

julia> @code_typed sincos(90u"°")
CodeInfo(
1 ─ %1 = Base.getfield(x, :val)::Int64
│   %2 = Base.sitofp(Float64, %1)::Float64
│   %3 = invoke Unitful.sind(%2::Float64)::Float64
│   %4 = invoke Unitful.cosd(%2::Float64)::Float64
│   %5 = Core.tuple(%3, %4)::Tuple{Float64, Float64}
└──      return %5
) => Tuple{Float64, Float64}
@sostock
Copy link
Collaborator

sostock commented Oct 30, 2024

Using the more accurate method would unfortunately hurt performance (apparently, sincosd is quite a bit slower than sincos):

julia> d = 20.0u"°";

julia> @btime cis($d);
  14.307 ns (0 allocations: 0 bytes) # current method: convert to unitless, call sincos
  33.159 ns (0 allocations: 0 bytes) # better accuracy: call sincosd

I still think we should use the more accurate sincosd. What do you think?

@jecs
Copy link
Author

jecs commented Oct 30, 2024

Personally, I like dispatching to the degree versions. The user can always uconvert to rad before calling these functions if better performance is required.

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.

2 participants