Remove the type constraint on sincosd

[singularitti]

Closes #41017

[singularitti]

I can't add test here, so I just paste the test in this PR:

julia> using Unitful, Test

julia> sincosd(5u"°")
ERROR: MethodError: no method matching sincosd(::Quantity{Int64, NoDims, Unitful.FreeUnits{(°,), NoDims, nothing}})
Closest candidates are:
  sincosd(::Real) at special/trig.jl:1249
  sincosd(::Missing) at special/trig.jl:1261
Stacktrace:
 [1] top-level scope
   @ REPL[1]:1

julia> function Base.sincosd(x)
           if isinf(x)
               return throw(DomainError(x, "sincosd(x) is only defined for finite `x`."))
           elseif isnan(x)
               return (oftype(x,NaN), oftype(x,NaN))
           end

           # It turns out that calling those functions separately yielded better
           # performance than considering each case and calling `sincos_kernel`.
           return (sind(x), cosd(x))
       end

julia> sincosd(5u"°")
(0.08715574274765818, 0.9961946980917455)

julia> @test sind(5u"°") == sind(5) == 0.08715574274765818
Test Passed

julia> @test cosd(5u"°") == cosd(5) == 0.9961946980917455
Test Passed

[stevengj]

Wouldn't it make more sense to change the type to ::Number?

[singularitti]

Wouldn't it make more sense to change the type to ::Number?

sincos has a method that applies to ::Any.

[stevengj]

sincos has a method that applies to ::Any.

Yes, but that method actually works for Any. For example, it works fine for sincos(A) where A is a square matrix.

The method defined here will only ever work for numbers.

[singularitti]

Ok, I think Number sounds fine.

[imciner2]

sincos has a method that applies to ::Any.

Yes, but that method actually works for Any. For example, it works fine for sincos(A) where A is a square matrix.

The method defined here will only ever work for numbers.

Didn't we get sind and cosd for square matrices in PR #39758 that was merged? The sincosd function here just calls sind and cosd with the argument, so I would think it should work for square matrices as well.

[stevengj]

Now that the function has been redefined to just call through to (sind(x), cosd(x)) with no other computations, I agree that Any makes sense.

[sostock]

I can't add test here

You could add tests with Complex numbers and/or matrices.

[singularitti]

I can't add test here

You could add tests with Complex numbers and/or matrices.

I actually want to add tests against Unitful degrees. I don't know whether I can call external libraries like Unitful.

[sostock]

I actually want to add tests against Unitful degrees. I don't know whether I can call external libraries like Unitful.

No, we don’t use Unitful in the tests (and I don’t think it makes sense to test the functionality provided by Unitful in the Base tests). You could use the Furlongs module (you would have to define sind(::Furlong{0}) and cosd(::Furlong{0})), but since sincosd just calls sind/cosd, I think it suffices to test that it works for other types in Base that have methods for sind/cosd.

[singularitti]

Can someone tell me should sind(5u"°") and sind(5) return the same results? We are having different opinions here.

[oscardssmith]

I'm pretty sure that sin(5u"°") == sind(5)is what we want (note the first is sin not sind).

[singularitti]

I'm pretty sure that sin(5u"°") == sind(5)is what we want (note the first is sin not sind).

What about sind(5u"°")?

[oscardssmith]

I think sind(5u"°")==sin(5u"°") since sind's documentation says that it "Compute sine of x, where x is in degrees." and 5u"°" is already in degrees. That said, I don't think you should ever call sind(5u"°"), and that it possibly should throw a methodError. If you want sin in degrees, and you have a type for creating numbers in degrees, that seems like the right way to do this.

In fact, sind and friends are arguably fairly un-Julian API. It's much more natural to define a type for degrees, and make sin work on it, than have 12 extra functions that conceptually do the same thing. These functions were added in 2013 (Julia 0.2), which was well before the ecosystem existed, and before some of the best practices for effectively using multiple dispatch were solidified. We probably can't delete these functions for 2.0, but I would kind of like to.

[singularitti]

It's much more natural to define a type for degrees

I agree. There should be a native Julia type for Degree.

[stevengj]

See #26362 for discussion of a Degree type.

[oscardssmith]

That's unfortunate. I wonder if it's still possible to do this.

see comments above