Accept only Number as input for sincos

[ronisbr]

With the old definition, it was possible to call sincos with Char inputs, which is not defined for sin and cos. Hence, we should accept only Real variables as input for sincos.

I put this in a separated PR because this is a breaking change (kind of...).

[ronisbr]

By the way, am I suppose to write the entry in NEWS or is it done in another step?

[andyferris]

What happens for a square matrix?

[StefanKarpinski]

What happens for a square matrix?

Related: #27254.

[ronisbr]

Oh, indeed sincos can accept arrays. I think I need to change that to match those:

[1] sin(x::BigFloat) in Base.MPFR at mpfr.jl:683
[2] sin(::Missing) in Base.Math at math.jl:1056
[3] sin(a::Complex{Float16}) in Base.Math at math.jl:1005
[4] sin(a::Float16) in Base.Math at math.jl:1004
[5] sin(z::Complex{T}) where T in Base at complex.jl:760
[6] sin(x::T) where T<:Union{Float32, Float64} in Base.Math at special/trig.jl:30
[7] sin(x::Real) in Base.Math at special/trig.jl:53
[8] sin(A::LinearAlgebra.Hermitian{#s548,S} where S<:(AbstractArray{#s549,2} where #s549<:#s548) where #s548<:Complex) in LinearAlgebra at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/symmetric.jl:720
[9] sin(A::Union{Hermitian{#s549,S}, Symmetric{#s549,S}} where S where #s549<:Real) in LinearAlgebra at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/symmetric.jl:716
[10] sin(D::LinearAlgebra.Diagonal) in LinearAlgebra at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/diagonal.jl:424
[11] sin(A::AbstractArray{#s549,2} where #s549<:Real) in LinearAlgebra at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/dense.jl:770
[12] sin(A::AbstractArray{#s549,2} where #s549<:Complex) in LinearAlgebra at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.0/LinearAlgebra/src/dense.jl:777

right?

[tkoolen]

With the old definition, it was possible to call sincos with Char inputs, which is not defined for sin and cos

Why is this a bad thing in itself? In fact, I'd go in the other direction and have the fallback for sincos just be

sincos(x) = (sin(x), cos(x))

I.e., none of this float business that causes issues like #26552. If sin and cos are not defined for x, a MethodError is thrown, which should be clear enough.

[ronisbr]

@tkoolen good point! I will do some tests to see if this does not cause any other problem.

[ronisbr]

Hi guys, sorry for the delay.

In fact @tkoolen I found a good explanation why removing the float is not good. Let's say you want to compute sincos(1) or sincos(π). In this case, since neither are Union{Float32, Float64}, it will fall back to (sin(x), cos(x)) which is slower. Hence, all numbers that can be converted to floats should call the faster version.

After thinking about it, the algorithm I am proposing in this commit is:

1. If the input is Float32 or Float64, call sincos to simultaneously compute the sine and cosine faster.
2. If the input is Number, try to convert it to float.
   a. If the converted Number is float, call sincos that will simultaneously compute the sine and cosine faster.
   b. Otherwise, compute the sine and cosine separately. This is the case if the input is Complex.
3. If the input is everything else, compute the sine and cosine separately. Hence, if the input type is not accepted by the functions sin and cos, then an error will be printed.

Is this good? Notice that it will work on every input supported by sin and cos and if the Number can be converted to and AbstractFloat, then the faster version is used. Makes sense?

[JeffBezanson]

Yes that sounds good.

[pkofod]

I'm not sure I follow the prioritized list. If <:Number has a sin and cos implementation, wouldn't most people expect it to use those rather than convert to float first?

[ronisbr]

If you do not convert an Int to Float then sincos will be like calling sin and cos separately. However, this is slower and, normally, people using sincos want to compute both faster.

[pkofod]

If you do not convert an Int to Float then sincos will be like calling sin and cos separately. However, this is slower and, normally, people using sincos want to compute both faster.

What if I have sin(::MyNumber) and cos(::MyNumber) defined. This is what I'm talking about. Then sincos does no longer do the safe thing of calculating sincos(x::MyNumber) = (sin(x), cos(x)) but rather it converts x through float(x) and returns a (most likely) Float64. It's not a deal-breaker. Number-definers just have to be aware that they need to define a specific sincos themselves, or their numbers will be converted through float instead of doing the separate calls.

[ronisbr]

Ah I see, sorry. Hum, I do not think about an easy way to correct this while keeping the current behavior. The only thing that I came up with was to add another specialization of sincos with every Number that can be converted to an AbstractFloat. This way, we can somewhat fix that @pkofod mentioned. What do you think?

[pkofod]

Hm , just don’t convert anything ?

[ronisbr]

In this case sincos(1) will be way slower than sincos(1.0). The same will apply for sincos(π):

julia> @benchmark sincos(1.0)
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     10.848 ns (0.00% GC)
  median time:      11.129 ns (0.00% GC)
  mean time:        11.251 ns (0.00% GC)
  maximum time:     46.524 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     999

julia> @benchmark sincos(1)
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     10.873 ns (0.00% GC)
  median time:      11.130 ns (0.00% GC)
  mean time:        11.273 ns (0.00% GC)
  maximum time:     46.574 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     999

julia> sincos(x::Int) = (sin(x),cos(x))
sincos (generic function with 9 methods)

julia> @benchmark sincos(1)
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     13.738 ns (0.00% GC)
  median time:      13.769 ns (0.00% GC)
  mean time:        13.886 ns (0.00% GC)
  maximum time:     44.532 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     998

[thchr]

What's a use case for calling sincos with ::Integer arguments?
It seems to me that cases like sincos(pi:: Irrational) or sincos(1) would only arise for literal calls, where speed wouldn't really matter anyway.
If someone wants to call sincos repeatedly with non-literal integer or irrational arguments (which, however, seems unlikely), they can just do sincos(float(x)).

[ronisbr]

But wouldn’t it be like a strange behavior from the user point of view?

Furthermore, it is not too uncommon. At my institute, we developed a satellite camera that has precisely 1 rad of FOV. You will see some code with sincos(theta) in which theta is integer and equal to 1. Having to make theta float to improve performance seems a little strange.

IMHO, the gain to eliminate this conversion to float is not too expressive. What do you think?

[StefanKarpinski]

Why would sincos be the only numerical function like this that accepts floats but not integers or other non-floating-point types? That seems like a non-starter.

[thchr]

It's not that it wouldn't accept non-floats, it's just that non-floats would go through the sincos(x) = (sin(x), cos(x)) fallback. You'd only have to add a float conversion if you wanted the speedup that sincos(x::Float64) gives. sincos(x) would still accept any input that have existing sin and cos implementations.

The real benefit of having methods besides sincos(x::Float64) = ... (+ possibly Float32) and sincos(x::Any) = (sin(x), cos(x)) doesn't seem clear? If I understand correctly, then this is also @tkoolen's & @pkofod's argument?
Any minor speedup attainable by converting everything to floats doesn't seem worthwhile for the cases mentioned (integer or irrational inputs).

[ronisbr]

Another argument I would give is that sin and cos also convert to float:

    sin(x::Real) = sin(float(x))

Hence, since it is already done, I think it is better to also do this here and call an optimized method. The case of @pkofod, in which a new type of number is created, seems more restricted than someone using this function with an integer input.

[tkoolen]

To summarize, when sincos is not specialized for some type, it's currently a choice between:

1. getting an error (either a MethodError or a StackOverFlowError as in Fallback math methods for ::Real are likely to call themselves #26552), or
2. silently not getting a performance improvement over (sin(x), cos(x)), but getting the correct result back.

Does anybody see a way to have the cake and eat it too?

[tkoolen]

Maybe something like

function sincos(x)
    if applicable(float, x)
        x′ = float(x)
        if typeof(x) === typeof(x′)
            # catch the stack overflow from #26552
            throw(ArgumentError("Calling float on `x` did not change its type."))
        end
        return sincos(x′)
    else
        return (sin(x), cos(x))
    end
end

but with tightened type signatures for float (get rid of the fallback that just calls AbstractFloat(x)). Alternatively, get rid of float entirely and just check for the availability of a conversion to AbstractFloat.

Another argument I would give is that sin and cos also convert to float:

sin(x::Real) = sin(float(x))

which I think should also be changed, perhaps in a manner similar to what I proposed above.

[ronisbr]

Hi @tkoolen ,

Now I think I am seeing the problem. Your solution seems interesting but we need to change a little bit because it will throw an error with Complex{Float[32,64]}, since it does not change after float and it is not AbstractFloat.

[ronisbr]

To leave things simple, in sincos, why not just do something like:

sincos(x::T) where T<:Union{Integer, Rational, AbstractIrrational} = sincos(float(x))
sincos(x) = (sin(x), cos(x))

I cannot recall any other type in which we would like to convert it directly to float to call the specialized method.

This will solve my problem and the one mentioned by @tkoolen and @pkofod . Then, if you want, I can start to think about changing that float in other trigonometric functions.

EDIT: In fact, couldn't we do this for all other trigonometric functions that do this conversion to float (without the fall back)? No, we could not...

[ronisbr]

Guys,

Although this last option is cleaner, it cannot be used this way. The file irrationals.jl is included after the math.jl in sysimg.jl. Hence, if I use AbstractIrrational here I get an error while building the sysimg:

LoadError("sysimg.jl", 339, LoadError("math.jl", 1061, LoadError("special/trig.jl", 201, UndefVarError(:AbstractIrrational))))

Question: should I let this as it is or can I move the irrationals.jl to be included before the math.jl in sysimg.jl?

[vtjnash]

discussion seems to have died