JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.46k stars 5.46k forks source link

`imag(pi)` gives an error #31949

Closed perrutquist closed 3 years ago

perrutquist commented 5 years ago

Attempting to take the imaginary part of an Irrational currently gives this error:

julia> imag(pi)
ERROR: MethodError: no method matching Irrational{:π}(::Int64)
Closest candidates are:
  Irrational{:π}(::T<:Number) where T<:Number at boot.jl:718
  Irrational{:π}() where sym at irrationals.jl:18
  Irrational{:π}(::Complex) where T<:Real at complex.jl:37
  ...
Stacktrace:
 [1] convert(::Type{Irrational{:π}}, ::Int64) at ./number.jl:7
 [2] oftype(::Irrational{:π}, ::Int64) at ./essentials.jl:372
 [3] zero(::Irrational{:π}) at ./number.jl:265
 [4] imag(::Irrational{:π}) at ./complex.jl:78
 [5] top-level scope at REPL[1]:1

julia> versioninfo()
Julia Version 1.3.0-DEV.167
Commit bdffb564d5 (2019-05-06 13:58 UTC)

A straightforward remedy would be to define imag(::Irrational) = 0.0

Alternatively, defining zero(::Irrational) = 0.0 would fix this issue, and at the same time fix the issue discussed in this thread: https://discourse.julialang.org/t/complexifying-pi/23885 This is the solution I'd personally prefer. However, I am not sure whether 0.0 constitutes an "additive identity" in this case, since adding it causes loss of precision.

(For more missing methods, see #26701)

For relevant discussion see: #21204, #22928

KristofferC commented 5 years ago

There exists an almost unbounded number of methods that are possible to add to ::Irrational. Manually adding a bunch of special cases and versions like foo(x::Irrational) = foo(float(x)) is in my opinion not a good idea.

Instead, it would be better to document that the Irrational interface is intentionally kept small, and for anything complicated you need to yourself convert it to a standard numeric type.

perrutquist commented 5 years ago

I disagree. The number is definitely bounded and roughly equal to the number of functions in Base that take Real arguments. (Almost all of which already work.)

(Note that the two-argument functions don't require a square number of methods to be added. Two different Irrationals will (nearly always) promote to Float64, so it's only necessary to add the method where all arguments are the same Irrational.)

Adding these methods requires less than one line of code per method (plus tests), on average. For example, all of the two-argument methods listed in #26701 can be created by making an existing line slightly longer.

The problem is not that users are likely to do things like imag(pi) explicitly, but it is rather common to have test loops like:

for x in (false, 1+im, 2, 3.0, pi, big(4), big(5.0), big(6.0)+im)
    # test foo(x), which should work with any Number
end

Such tests should not fail simply because a function in Base does not support a type that is in Base.

JeffreySarnoff commented 5 years ago

These definitions would enable many interoperables mentioned here and elsewhere.

Base.complex(x::Irrational{S}) where {S} = complex(x, 0.0)

Base.imag(x::Irrational{S})  where {S} = 0.0
Base.angle(x::Irrational{S}) where {S} = 0.0
dalum commented 5 years ago

Just ran into a similar issue with negative powers:

julia> pi^-1
ERROR: MethodError: no method matching Irrational{:π}(::Int64)

EDIT: Gah, sorry! This has been fixed, I was on 1.1.0 :facepalm:.

JeffreySarnoff commented 5 years ago

that is expected ( precedence )

julia> pi^(-1)
0.3183098861837907
StefanKarpinski commented 5 years ago

I'm starting to get real itchy to delete pi altogether.

JeffreySarnoff commented 5 years ago

please do -- maybe first move it to a new module "Base.\<shifted space>"

perrutquist commented 5 years ago

One thing that the above-mentioned complex(pi), imag(pi), angle(pi), and pre- #31068 pi ^ -1 have in common is that the would all work if the additive and multiplicative unities were defined for Irrational numbers. In other words, these would all be fixed by these three lines:

Base.zero(::Irrational) = 0.0
Base.zero(::Type{<:Irrational}) = 0.0
Base.one(::Type{<:Irrational}) = 1.0

Other functions that would also be fixed by the above are: acot(pi), acotd(pi), acoth(pi), acsc(pi), acscd(pi), acsch(pi), asec(pi), asecd(pi), Complex(pi), cot(pi), cotd(pi), coth(pi), csc(pi), cscd(pi), csch(pi), modf(pi), reim(pi), sec(pi), secd(pi), sech(pi) sign(pi) and likely a number of user-defined functions in the wild.

The documentation does not require zero(T) and one(T) to be of the type T, but for the type. (Different story for oneunit.) However 0.0 and 1.0are not technically the additive and multiplicative identity as they cause conversion to Float64.

One option would be to use false and true instead of 0.0 and 1.0 and define things like *(x::Bool, y::Irrational) = x ? y : 0.0, making one(pi) return the true multiplicative identity. (I believe that particular one can be done by deleting one existing line of code.) It would be slightly breaking in case somebody is currently using true*pi as a way of forcing conversion to Float64.

StefanKarpinski commented 5 years ago

Maybe we should define this instead:

Base.zero(::Irrational) = false
Base.zero(::Type{<:Irrational}) = false
Base.one(::Type{<:Irrational}) = true

That would reduce the amount of 64-bit poisoning that happens?

perrutquist commented 5 years ago

Yes. That was what I was trying to say in the last paragraph of my previous comment.

perrutquist commented 5 years ago

Two more methods that definitely need fixing (unless pi is removed altogether) are sin(pi) and tan(pi). These are not just off by a few ULPs. - All of the returned digits are wrong.

StefanKarpinski commented 5 years ago

We're not going to remove pi since that would be very breaking, I'm just cranky that we seem to have an endless supply of issues about missing functionality for the Irrational type which was never meant to be computational in the first place.

perrutquist commented 5 years ago

I had guessed as much. I'd be happy to make a PR with the above-mentioned zero and one methods, plus two-argument methods for cld, div, divrem, fld, mod, mod1 and rem (c.f. #26701)

After that, there should not be many problematic functions left in Base. A naïve for loop, testing for functions that accept 3.14 but not pi only lists functions like eps and nextfloat that do not make sense for Irrational.

thisrod commented 5 years ago

the Irrational type which was never meant to be computational in the first place.

Given that, how is one supposed to construct Float64(π) for computational purposes? The docstring for Irrational only says: "Number type representing an exact irrational value". I would write an expanded version, except that I don't know how it's supposed to be used.

I found this issue when I discovered that Complex(pi) throws a MethodError, and Complex{Real}(pi) triggers an infinite recursion, but π ≈ Complex{Real}.([1,π])[2] returns true. Is that expected? If not, should I raise it an a separate issue?

JeffreySarnoff commented 5 years ago
julia> pi = Float64(π)
3.141592653589793
julia> 2pi
6.283185307179586

Real is not a concrete type, it is an abstract type. Complex{T} expects T<:Real not T === Real .

vtjnash commented 3 years ago

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 and not add more methods to them. Just worth noting that the links in the relevant discussion were closed for the same reason in years past, so we seem to be pretty consistent here still in our thinking.