Skip to content

Add Irrational support to rem2pi #43539

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add Irrational support to rem2pi #43539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0231547
Add Irrational support to rem2pi
garrison Dec 24, 2021
ee83f2f
Adjust `rem2pi(x::Real, r)` implementation to match other trig functions
garrison Jan 16, 2022
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
9 changes: 7 additions & 2 deletions base/math.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,9 +1202,14 @@ end
rem2pi(x::Float32, r::RoundingMode) = Float32(rem2pi(Float64(x), r))
rem2pi(x::Float16, r::RoundingMode) = Float16(rem2pi(Float64(x), r))
rem2pi(x::Int32, r::RoundingMode) = rem2pi(Float64(x), r)
function rem2pi(x::Int64, r::RoundingMode)
function rem2pi(x::Integer, r::RoundingMode)
fx = Float64(x)
fx == x || throw(ArgumentError("Int64 argument to rem2pi is too large: $x"))
Copy link
Member

@JeffBezanson JeffBezanson Jan 5, 2022

Choose a reason for hiding this comment

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

I feel like we need a similar check --- if the conversion is inaccurate the answer might be completely wrong.

Copy link
Member Author

@garrison garrison Jan 20, 2022

Choose a reason for hiding this comment

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

Thanks. Based on your comment and some additional thinking on my part, I have added an additional commit to this PR: ee83f2f. I'm not sure that it actually addresses the fundamental issue you raise, but let me explain my reasoning.

When I first opened this PR, I thought it made sense to convert to a Float64, but I have since realized that calling float will work better more generally. This was motivated by thinking about how BigFloat should behave, even though it turns out that rem2pi(::BigFloat, r) is already defined in mpfr.jl, so this is not strictly needed for BigFloat. (Note that rem2pi is already defined for Float32 and Float16 just above the current block of code.)

Although this change does not result in a similar check to the integer case, it now puts rem2pi on the same footing as the ::Real implementation of many other functions, including trig functions which, like this one, accept radians as argument. I don't see any reason why the bar for accuracy should be higher for rem2pi than for those functions (think of tan, for instance, which has the same period but is more poorly conditioned because of the asymptotes).

fx == x || throw(ArgumentError("Integer argument to rem2pi is too large: $x"))
rem2pi(fx, r)
end
function rem2pi(x::Real, r::RoundingMode)
fx = float(x)
x === fx && throw(MethodError(rem2pi, (x, r)))
rem2pi(fx, r)
end

Expand Down
5 changes: 5 additions & 0 deletions base/mathconstants.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,9 @@ Base.sincos(::Irrational{:π}) = (0.0, -1.0)
Base.tan(::Irrational{:π}) = 0.0
Base.cot(::Irrational{:π}) = -1/0

Base.rem2pi(::Irrational{:π}, ::RoundingMode{:Nearest}) = π
Base.rem2pi(::Irrational{:π}, ::RoundingMode{:ToZero}) = π
Base.rem2pi(::Irrational{:π}, ::RoundingMode{:Down}) = π
Base.rem2pi(::Irrational{:π}, ::RoundingMode{:Up}) = -π

end # module
8 changes: 8 additions & 0 deletions test/numbers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2611,6 +2611,14 @@ end
@test rem2pi(T(-8), RoundUp) ≈ -8+2pi
end

@testset "rem2pi irrational" begin
@test rem2pi(π, RoundNearest) ≈ rem2pi(float(π), RoundNearest)
@test rem2pi(π, RoundToZero) ≈ rem2pi(float(π), RoundToZero)
@test rem2pi(π, RoundDown) ≈ rem2pi(float(π), RoundDown)
@test rem2pi(π, RoundUp) ≈ rem2pi(float(π), RoundUp)
@test rem2pi(ℯ, RoundNearest) ≈ rem2pi(float(ℯ), RoundNearest)
end

import Base.^
struct PR20530; end
struct PR20889; x; end
Expand Down