Skip to content

Conversation

@garrison
Copy link
Member

@garrison garrison commented Dec 24, 2021

This adds Irrational support to rem2pi and thus fixes #42799. One thing that is a bit awkward is that rem2pi(π, r) returns π in the form of an Irrational for each rounding mode except RoundUp, where it instead returns the Float64 equivalent of . I don't expect this will lead to type instability in practice, as typically the rounding mode is fixed in the code and is thus known at compile time.

I also added a version that accepts any Real, so that e.g. the exponential constant ℯ can be passed. Admittedly, I don't see any real use case for calling with that constant in particular, but at least it works as expected now, and there are likely other, more useful, cases that will benefit from this. It also follows the practice of other methods (e.g. sin, cos), which do indeed accept this constant by converting it to a float. While making this change, I also changed the existing method specializing on Int64 to specialize on Integer instead, as that method will work for any Integer that can be exactly converted to a Float64, whether it is an Int16, Int32, Int64, or even a BigInt.

@ViralBShah ViralBShah added the maths Mathematical functions label Dec 24, 2021
end
function rem2pi(x::Real, r::RoundingMode)
fx = Float64(x)
fx == x || throw(ArgumentError("Int64 argument to rem2pi is too large: $x"))
Copy link
Member

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

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

@brenhinkeller brenhinkeller added the feature Indicates new feature / enhancement requests label Nov 19, 2022
@jamblejoe
Copy link
Contributor

What is missing for this PR to be merged? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Indicates new feature / enhancement requests maths Mathematical functions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rem2pi not defined for Irrational

5 participants