[DO NOT MERGE] make sinpi/cospi return Int

[simeonschaub]

ref #35820. There was some discussion on Slack about how breaking this change would be, so could someone with permission trigger PkgEval on this? I think it would be good to get a better understanding of how much code actually relies on the specific return type. Of course, this won't catch performance regressions, e.g. due to dispatching on generic_matmul instead of BLAS, so we might still want to be careful, even if PkgEval is successful.

[jonas-schulze]

Should the return type match the input, i.e. zero(x)?

[simeonschaub]

See my other comment. I think for consistency, the return type of sinpi should always be the same as cospi, though, even if we could actually return Unsigned here.

[jonas-schulze]

I think it's fine if they differ, so that some computations remain Unsigned if all their underlying functions allow it. If someone plugs results of sinpi and cospi together, type promotion will fix that difference.

[giordano]

[DO NOT MERGE]

If you don't want a pull request to be merged, just make it draft, so you also prevent someone from accidentally hitting the button 😉 You can also turn to draft after opening the pull request, there is a button on the right

[simeonschaub]

Thanks! I thought draft PRs could only be created by members. Maybe they changed that.

[giordano]

I'm still unconvinced that this is a good change. I'd argue that the fact that cospi(::Integer) is the inverse of signbit is just an accident. This is certainly an optimisation, but is it worth the effort? How often sinpi and cospi have Integer arguments in real-world uses? If I know that I'm going to use Integers with these functions it's likely I will just not use them.

[Keno]

We do generally preserve integerness when a function maps integers to integers. I don't necessarily see why this should be any different.

[jakobnissen]

I pushed back hard on this on Slack, so I'd thought I'd write it here as well so my objections don't just disappear into the Slack memory hole.

Making this function return an Int (or even UInt) would indeed be best. My issue with this is merging it in the 1.x releases, since I consider this a breaking change. Worse still, it is a breaking change that have very little benefit, since you can always just convert to Int in your own code (or pirate the function if you really want the function to return an Int).

Why is it breaking, if Ints promotes automatically to Float64? It's breaking for two reasons:

1. A container e.g. Vector{Int64} cannot contain float values. This matters if e.g. a user does something like

function foo(itr)
    vector = [sinpi(i) for i in itr]
    return mean!(vector)
end

This will not work any more.

2. As Julians, we extol our neat multiple dispatch. Having multiple dispatch in a language absolutely means that changing the return type of a function means its behaviour is modified. E.g. if you have

step_one(x::Int64) = 2*sinpi(x) + 1
step_two(y::Float64) = asin(x)
all_steps(x::Int64) = step_two(step_one(x))

This will suddenly fail with this "minor change".

Ultimately, I don't think we can just run PkgEval to see any user code it breaks happens to be in a package. Most of my Julia code is not uploaded as a package. Certainly most commercial code is not a public available package. This change could very easily break code we don't even know exists.

Also, this is not some obscure corner case of the language, like relying on Base internals or Core.compiler or the behaviour of the optimizer. This code breaks regular Julia code using regular functions. If this code could break, what code can't break in minor versions?

[jonas-schulze]

@jakobnissen I'm 100% with you that there is a lot of code outside of public packages that shouldn't break by a Julia upgrade, unless it hasn't been "broken" before. I was happy to see this PR since I had the same thoughts when reading #33341. Hence I consider this more of a fix rather than a breaking feature. Some code will break, that's without question (to me), but has that code been "correct" in the first place?

1. A container e.g. Vector{Int64} cannot contain float values. This matters if e.g. a user does something like

function foo(itr)
    vector = [sinpi(i) for i in itr]
    return mean!(vector)
end

I assume you mean Statistics.mean!. If it is used like

d = [1.0] # note the float value here
v = rand(Int, 10)
mean!(d, v)

then everything is just fine. This would only break if you would use your vector as the first argument of mean!, but I don't understand why one would initialize that vector first other than to reuse some vector. This, however, feels more like an optimization relying on internals, which I'd argue is broken already.

2. As Julians, we extol our neat multiple dispatch. Having multiple dispatch in a language absolutely means that changing the return type of a function means its behaviour is modified. E.g. if you have

step_one(x::Int64) = 2*sinpi(x) + 1
step_two(y::Float64) = asin(x)
all_steps(x::Int64) = step_two(step_one(x))

As there is no multiple dispatch in this example, what was your intent? Since asin is perfectly fine with integer arguments, why did you assert types in the first place?

If it was meant as a readable version of

step(x::Int64) = 2*sinpi(x) + 1
step(y::Float64) = asin(x)
all_steps(x::Int64) = step(step(x))

then I'd say that is a code smell, i.e. broken already.

Would you mind to elaborate a bit more on your examples?

[jakobnissen]

@jonas-schulze Yes, let me be more precise.

I don't think it's useful to argue with precise examples of code that would break, because it's very hard to predict what kind of code is in packages we don't know about. What we can argue about is whether useful functionality could be written that depends on sinpi returning Float64, and whether this functionality could be written using standard Julia language features, i.e. no experimental/obscure/nondocumented/unexported features.

My first example highlights that

• There are functions that necessarily have to take a container of floats, like mean!
• Changing the type of sinpi to Int will change the type of a container storing the result to Int, meaning
• This will break any combination of container + function that has to take a container of floats.

A more realistic example may be a situation where a container is initialized with sinpi(::Integer) and then elements of sinpi(::Float64) are pushed to the container. But again, I'm sure code will contain very reasonable examples we can't predict. What I show is merely that it's very easy to come up with example code that this will break.

You are right that my second example does not show multiple dispatch. It does show strict type annotation in functions. Nonetheless, type annotations is absolutely a thing, e.g. I use it all the time for internal function as a way of documenting what input and output types are expected to catch errors early when developing. One could argue that adding specific type annotations in functions is not the best coding style. That's irrelevant. I like doing it and think it help my development, and in any case, arguing that we can break users' code if we consider their coding style to be poor is a complete non-starter.

You are probably right that multiple dispatch is not relevant here. It's hard to imagine a function that does one thing when given an integer and something completely different when given a float. But as mentioned earlier, it's not hard to imagine a function that simply won't work with integers, but will with floats.

[simeonschaub]

Would it make sense to move this discussion to #35820? I do understand both sides of the argument and am also not 100% sure, this still counts as "minor breaking". The reason I opened this PR was to get a sense of how much code would actually be affected by this. I do understand that just because something passes PkgEval, does not mean this change won't break any other code, but I do think it should be quite representative of real world code. With pretty much every change that is made to Julia, you can of course construct code examples that this change would break, but what I am more interested in is whether this breaks code patterns that are commonly used.

[jonas-schulze]

@jakobnissen thanks for explaining, now I got your point. I too like and use type annotations a lot, often times too strict ones too. Breaking stuff because one might consider someone elses coding style poor hasn't been part of my reasoning; I'm not adept enough to judge. I don't want to offend anyone. If instead things break because one hasn't yet fully understood it, is IMHO just part of the learning experience. If Julia would be 100% enterprise, there wouldn't even be "minor breaking" changes.

[jonas-schulze]

I think it's fine if sinpi and cospi have different return types, so that some computations remain Unsigned if all their underlying functions allow it. If someone plugs results of sinpi and cospi together, type promotion will fix that difference.

[oscardssmith]

Is the optimizer smart enough to remove the branch for toes where zero=-zero?

[jonas-schulze]

I think I don't know what you mean. For integers there (usually?) is no difference between +0 and -0.

[simeonschaub]

I still think sinpi and cospi are too closely related to return different types. They are often used together, for example in cispi #35792 and sincospi #35816. Type promotion doesn't address all those problems, since the latter would produce an inhomogenous typed tuple and could produce quite unexpected and hard to debug results in edge cases. What would be the benefit of returning unsigned integers instead?

[jonas-schulze]

It seems like type promotion as quiete preservative with UInts already, so I'm ok with your solution.

julia> typeof(UInt8(10) - Int8(2)) == typeof(UInt8(10) + Int8(2)) == UInt8
true

[PallHaraldsson]

[Would we only merge this for purists, and this method not ever used in practice, as "useless"?]
@jonas-schulze, Julia assumes all platforms use two's complement integers (like current C++, unlike C and older C++), https://en.wikipedia.org/wiki/Ones'_complement is very outdated, but given such a type (and with Julia you could make one, I'm not sure why you would), I'm not even sure if it should do the similar to floats:

julia> sin(-0.0)
-0.0

julia> sin(-0.0) == sin(-0)  # defined true, even with returning opposite signs (as -0 = 0):
true

[Does -0 == +0 usually, on the ancient one's complement platforms?]

[simeonschaub]

@PallHaraldsson That does sound like a very hypothetical example to me. I am not aware of Julia running on any non-two's-complement architectures. This assumption is also made throughout all of Base, so if there was such a platform, these would probably be the least of our worries.

[PallHaraldsson]

Right, I was just addressing his "(usually?) is no difference between +0 and -0." I'm not worried about any of this, and would be ok with status quo, just like division returns floats (as natural at least there). Your change would work with all integer types, I know already defined, e.g. BitIntegers.jl, and all future types hopefully (like if someone made up one's complement on two-complements architecture).

[eschnett]

What about cos(pi), sin(pi), and tan(pi)? With typeof(pi) = Irrational{:π}, one can argue that these should also be special-cased.

log(ℯ)? (Oh well, that one already returns an integer.)

exp2(Unsigned(4))?

[simeonschaub]

I believe the current behavior for sin(pi) is intentional, since otherwise it would be really confusing that e.g. sin(2pi) behaved differently. We generally try not to implement too much "magic" with Irrationals, because at some point, you are basically implementing a small CAS, which is not really the goal of Irrational.
The log(ℯ) case does indeed look dubious to me, there is a comment next to it that reads

use 1 to correctly promote expressions like log(x)/log(ℯ)

But that doesn't sound like a very convincing case to me.
exp2 does sound like it could make sense to special case for Unsigned, although one might argue that it is more closely tied to floating point values and therefore the current behavior makes sense.

[StefanKarpinski]

I think the point here is that the current policy of always returning floats for these kind of mathematical functions, even if it's in principle possible to return integers for certain inputs, is easy to understand and predict. If we start making exceptions then it becomes much harder to reason about which functions and what circumstances are exceptional. What's the concrete benefit of returning integers for sinpi and cospi with integer inputs?

[eschnett]

The original idea was to come up with a convenience function that calculates (-1)^n efficiently, as a kind-of inverse to signbit. While bikeshedding, we discovered that this happens to be exactly the cospi function. What a coincidence! Certainly worth exploring. At the moment, I agree that we should leave cospi / sinpi as they are and just invent a new function name.

[simeonschaub]

The main benefit I see in returning integers is that it avoids unnecessary widening types. Returning Float64 for all integer input types doesn't seem that useful as a feature, but in generic code this could lead to Float32s being converted to Float64 accidentally. cospi with integer inputs can be quite useful, because it has some useful relationships with DFTs. Where I have needed this before was for calculating structure factors. In my case I ended up calculating everything in double precision anyways, but if I wanted to run this on a GPU, it would have been quite difficult to write this in a generic way that things don't get converted from Float32 to Float64 in between. I don't think there are many functions that currently return Float64 if their output is always integer for integer inputs, except for the cases @eschnett pointed out, so I don't think it's unreasonable for users to expect <:Integer as output type if the output of a function can only ever be a whole number. My main worry about this change is that it would be a breaking change in some cases.