RFC: More methods for Irrational #32117

perrutquist · 2019-05-23T05:38:57Z

The purpose of this PR is to make Irrational work with many more functions, including those mentioned in #31949 #26701 and #22878.

The main way that this is accomplished is by defining

zero(::AbstractIrrational) = false
zero(::Type{<:AbstractIrrational}) = false
one(::Type{<:AbstractIrrational}) = true

The documentation says that these functions should return the additive/multiplicative identities for the type T - not of the type T.

In order to make false and true into additive/multiplicative identities, this PR also re-defines Irrational-Bool addition, subtraction and multiplication.
This is technically breaking if someone is currently using true * pi or pi + false as a way of forcing conversion to Float64. (The alternatives 1 * pi and 0 + pi continue to work as before.)
The new +, - and * methods are not type-stable, which the old ones were. On the other hand, Julia is now much better at type inference than it was when the original methods were written.

With this, a whole range of functions start to work, as the only reason they were throwing before was a call to one or zero. To give one example, sec(x::Number) is defined as one(x)/cos(x) which now works.

This PR also extends the list of two-argument functions that convert to Float64 when given two identical Irrational arguments. The intent is to make all "math-like" functions in Base work with Irrational. (This should mitigate the effect of true*pi now returning pi rather than a Float64.)

Finally, some minor fixes: widemul of two Irrationals now returns a BigFloat same as widemul of Float64s, and sin(pi) and tan(pi) now return exactly 0.0.

perrutquist · 2019-05-23T06:52:30Z

Another effect that should be mentioned is that complex(pi) (previously an error) will now return a Complex{Float64} because of how (pi, zero(pi)) is promoted.

perrutquist · 2019-05-23T07:05:09Z

...and for that reason it seemed to make sense to also make Complex(pi, pi) a Complex{Float64} rather than Complex{Irrational{:π}}. It would be a very odd special case.

ararslan · 2019-05-23T19:18:08Z

The documentation says that these functions should return the additive/multiplicative identities for the type T - not of the type T.

I think that's taking the documentation a bit overly literal in this case. I don't know of any other cases where zero/one does not return value of the input type. Indeed, it's assumed in a number of places, especially when determining the element type for a container.

stevengj · 2019-05-23T20:21:03Z

I don't know of any other cases where zero/one does not return value of the input type.

oneunit(T) usually returns something of type T, but one(T) does not for dimensionful types.

mohamed82008 · 2020-02-15T16:47:15Z

I am running into issues with DiffRules and Distributions that this PR can fix. Are people here for or against the proposed changes? If only a rebase is needed, I can go ahead and do that.

mohamed82008 · 2020-02-15T16:48:25Z

I only care about one and zero for irrationals.

stevengj · 2020-02-15T23:27:24Z

base/irrationals.jl

+Rational(x::AbstractIrrational) = rationalize(x)
+rationalize(x::Irrational{T}) where T = throw(ArgumentError("Type must be specified. Use rationalize(Int, $T) to obtain a Rational{Int} approximation to the irrational constant $T."))
+Math.mod2pi(x::AbstractIrrational) = Float64(mod2pi(big(x)))
+Math.rem2pi(x::AbstractIrrational, m) = Float64(rem2pi(big(x), m))


Shouldn't use BigFloat arithmetic at runtime.

Good point. I will turn these into generated functions.

stevengj · 2020-02-15T23:28:41Z

I'm okay with one and zero. The other functions are mostly fine but might require some more thought. My suggestion would be to make a separate PR with just one and zero if that's all you need for now, since that can be merged quickly.

mohamed82008 · 2020-02-15T23:42:16Z

Sounds good, will do.

mohamed82008 · 2020-02-16T00:16:03Z

Done.

Define `zero(Irrational)` and `one(Irrational)` to return `false` and `true` respectively. (This is breaking if someone is currently using `true * pi` or `pi + false` as a way of forcing conversion to `Float64`.) Make `false` act as the additive identity for `Irrational` by defining a method for `+`. (Not type stable) Make `true` act as the multiplicative identity for `Irrational` by defining a method for `*` (Not type stable) Make `widemul` of two `Irrational`s return a `BigFloat` Make `sin(pi)` and `tan(pi)` return exactly 0.0 Define a number of methods on `Irrational` to default to conversion to Float64.

perrutquist · 2020-02-16T10:06:39Z

I'm very sorry. I tried to rebase this on the latest master, and I obviously messed up. ~~Now it looks like I've modified 1658 files. Git and I are not always friends. Will try to fix this.~~ Hopefully fixed now.

vtjnash · 2021-04-08T19:48:50Z

base/irrationals.jl

+@generated Math.mod2pi(x::AbstractIrrational) = Float64(mod2pi(big(x())))
+@generated Math.rem2pi(x::AbstractIrrational, r::RoundingMode) = Float64(rem2pi(big(x()), r()))


Suggested change

@generated Math.mod2pi(x::AbstractIrrational) = Float64(mod2pi(big(x())))

@generated Math.rem2pi(x::AbstractIrrational, r::RoundingMode) = Float64(rem2pi(big(x()), r()))

Math.mod2pi(x::AbstractIrrational) = Float64(mod2pi(big(x)))

Math.rem2pi(x::AbstractIrrational, r::RoundingMode) = Float64(rem2pi(big(x), r))

JeffBezanson · 2021-04-08T19:49:57Z

base/irrationals.jl

    @eval $op(x::AbstractIrrational, y::AbstractIrrational) = $op(Float64(x),Float64(y))
 end
-*(x::Bool, y::AbstractIrrational) = ifelse(x, Float64(y), 0.0)
+*(x::Bool, y::AbstractIrrational) = x ? y : zero(y)


Type unstable --- I'm not sure this is worth it.

vtjnash · 2021-04-08T19:50:58Z

base/irrationals.jl

@@ -45,6 +51,7 @@ promote_rule(::Type{S}, ::Type{T}) where {S<:AbstractIrrational,T<:Number} = pro
 AbstractFloat(x::AbstractIrrational) = Float64(x)::Float64
 Float16(x::AbstractIrrational) = Float16(Float32(x)::Float32)
 Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x))
+Complex(x::AbstractIrrational, y::AbstractIrrational) = Complex(Float64(x), Float64(y))


Seems unnecessary?

julia> Complex(π, ℯ) 3.141592653589793 + 2.718281828459045im

Suggested change

Complex(x::AbstractIrrational, y::AbstractIrrational) = Complex(Float64(x), Float64(y))

vtjnash · 2021-04-08T19:54:34Z

base/irrationals.jl

+
+widemul(x::AbstractIrrational, y::AbstractIrrational) = big(x)*big(y)
+
+(::Colon)(x::AbstractIrrational, y::AbstractIrrational) = Float64(x):Float64(y)


Seems to already work without this?

Suggested change

(::Colon)(x::AbstractIrrational, y::AbstractIrrational) = Float64(x):Float64(y)

julia> ℯ:π 2.718281828459045:1.0:2.718281828459045

vtjnash · 2021-04-08T20:06:45Z

We discussed this on triage, and felt that it is better to be explicit about the transforms for irrational numbers into the appropriate computational types. Thanks for the PR however. I know it is sad to see one not get merged, but it happens to all of us.

StefanKarpinski requested review from stevengj and JeffBezanson May 23, 2019 16:19

mohamed82008 mentioned this pull request Feb 15, 2020

Implement FillDist and ArrayDist TuringLang/DistributionsAD.jl#19

Merged

stevengj reviewed Feb 15, 2020

View reviewed changes

mohamed82008 mentioned this pull request Feb 16, 2020

zero and one for irrational numbers #34773

Merged

perrutquist force-pushed the more_irrational branch from d3a83a7 to 020d8ee Compare February 16, 2020 10:09

move computation of mod2pi(::Irrational) to compile time

511a538

StefanKarpinski added the triage This should be discussed on a triage call label Feb 17, 2020

Merge branch 'master' into more_irrational

408c9af

vtjnash reviewed Apr 8, 2021

View reviewed changes

JeffBezanson reviewed Apr 8, 2021

View reviewed changes

vtjnash reviewed Apr 8, 2021

View reviewed changes

vtjnash closed this Apr 8, 2021

oscardssmith removed the triage This should be discussed on a triage call label May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: More methods for Irrational #32117

RFC: More methods for Irrational #32117

perrutquist commented May 23, 2019 •

edited

Loading

perrutquist commented May 23, 2019 •

edited

Loading

perrutquist commented May 23, 2019

ararslan commented May 23, 2019

stevengj commented May 23, 2019

mohamed82008 commented Feb 15, 2020

mohamed82008 commented Feb 15, 2020

stevengj Feb 15, 2020

perrutquist Feb 16, 2020

stevengj commented Feb 15, 2020

mohamed82008 commented Feb 15, 2020

mohamed82008 commented Feb 16, 2020

perrutquist commented Feb 16, 2020 •

edited

Loading

vtjnash Apr 8, 2021

JeffBezanson Apr 8, 2021

vtjnash Apr 8, 2021

vtjnash Apr 8, 2021

vtjnash commented Apr 8, 2021

		@generated Math.mod2pi(x::AbstractIrrational) = Float64(mod2pi(big(x())))
		@generated Math.rem2pi(x::AbstractIrrational, r::RoundingMode) = Float64(rem2pi(big(x()), r()))


		widemul(x::AbstractIrrational, y::AbstractIrrational) = big(x)*big(y)

		(::Colon)(x::AbstractIrrational, y::AbstractIrrational) = Float64(x):Float64(y)

RFC: More methods for Irrational #32117

RFC: More methods for Irrational #32117

Conversation

perrutquist commented May 23, 2019 • edited Loading

perrutquist commented May 23, 2019 • edited Loading

perrutquist commented May 23, 2019

ararslan commented May 23, 2019

stevengj commented May 23, 2019

mohamed82008 commented Feb 15, 2020

mohamed82008 commented Feb 15, 2020

stevengj Feb 15, 2020

Choose a reason for hiding this comment

perrutquist Feb 16, 2020

Choose a reason for hiding this comment

stevengj commented Feb 15, 2020

mohamed82008 commented Feb 15, 2020

mohamed82008 commented Feb 16, 2020

perrutquist commented Feb 16, 2020 • edited Loading

vtjnash Apr 8, 2021

Choose a reason for hiding this comment

JeffBezanson Apr 8, 2021

Choose a reason for hiding this comment

vtjnash Apr 8, 2021

Choose a reason for hiding this comment

vtjnash Apr 8, 2021

Choose a reason for hiding this comment

vtjnash commented Apr 8, 2021

perrutquist commented May 23, 2019 •

edited

Loading

perrutquist commented May 23, 2019 •

edited

Loading

perrutquist commented Feb 16, 2020 •

edited

Loading