Skip to content

[DO NOT MERGE] make sinpi/cospi return Int #35823

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[DO NOT MERGE] make sinpi/cospi return Int #35823

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions base/special/trig.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,8 +927,8 @@ function sincospi(x::T) where T<:Rational
end
end

sinpi(x::Integer) = x >= 0 ? zero(float(x)) : -zero(float(x))
cospi(x::Integer) = isodd(x) ? -one(float(x)) : one(float(x))
sinpi(x::T) where {T<:Integer} = zero(signed(T))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jonas-schulze jonas-schulze May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@PallHaraldsson PallHaraldsson May 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?]

Copy link
Member Author

@simeonschaub simeonschaub May 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

cospi(x::T) where {T<:Integer} = isodd(x) ? -one(signed(T)) : one(signed(T))
sincospi(x::Integer) = (sinpi(x), cospi(x))
sinpi(x::Real) = sinpi(float(x))
cospi(x::Real) = cospi(float(x))
Expand Down