Skip to content
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.

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

problem with typemin and typemax for Irrational #36978

Closed
bkamins opened this issue Aug 9, 2020 · 10 comments · Fixed by #50894
Closed

problem with typemin and typemax for Irrational #36978

bkamins opened this issue Aug 9, 2020 · 10 comments · Fixed by #50894
Labels
maths Mathematical functions types and dispatch Types, subtyping and method dispatch

Comments

@bkamins
Copy link
Member

bkamins commented Aug 9, 2020

I think it is a bug:

julia> t = typeof(pi)
Irrational{:π}

julia> hasmethod(typemin, (t,))
true

julia> typemin(t)
ERROR: MethodError: no method matching typemin(::Type{Irrational{:π}})

The same happens with typemax

@bkamins
Copy link
Member Author

bkamins commented Aug 9, 2020

@nalimilan - a little corner-case surprise

I do not know the internals of jl_gf_invoke_lookup for a general fix, but for Irrational probably we can just define typemax and typemin to return the singleton.

Now I also see that we have also a problem here:

julia> typemax(Real)
ERROR: MethodError: no method matching typemax(::Type{Real})
Closest candidates are:
  typemax(::Type{Pkg.Types.PackageMode}) at Enums.jl:191
  typemax(::Type{LibGit2.Consts.GIT_OPT}) at Enums.jl:191
  typemax(::Type{Int64}) at int.jl:726
  ...
Stacktrace:
 [1] top-level scope at REPL[54]:1

julia> hasmethod(typemax, (Real,))
true

@bkamins
Copy link
Member Author

bkamins commented Aug 10, 2020

Also while we are at it:

julia> zero(pi)
false

julia> zero(pi) + pi
3.141592653589793

do we think it is consistent?

@User-764Q
Copy link

Bumping this I tested today on Julia 1.6.2 and got a similar error.

julia> t = typeof(pi)
Irrational{}

julia> hasmethod(typemin, (t,))
true

julia> typemin(t)
ERROR: MethodError: no method matching typemin(::Type{Irrational{:π}})
Closest candidates are:
  typemin(::Union{Dates.DateTime, Type{Dates.DateTime}}) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/Dates/src/types.jl:427
  typemin(::Union{Dates.Date, Type{Dates.Date}}) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/Dates/src/types.jl:429
  typemin(::Union{Dates.Time, Type{Dates.Time}}) at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/Dates/src/types.jl:431
  ...
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1

julia> ```



`

@KristofferC
Copy link
Member

There is Base.hastypemax which is probably the more correct thing to do. hasmethod is not really useful in normal code, as shown by this example.

@brenhinkeller brenhinkeller added bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch labels Nov 20, 2022
@brenhinkeller
Copy link
Contributor

Still reproduces on 1.8.3

@brenhinkeller brenhinkeller added the maths Mathematical functions label Nov 20, 2022
@nsajko
Copy link
Contributor

nsajko commented Aug 2, 2023

I don't think this is connected to typemin or typemax at all, it's just how hasmethod works. The issue seems to be that people expect hasmethod to work recursively, while this is not the case, even though it would be more "user-friendly".

For example:

julia> h(::T) where {T} = hasmethod(typemin, Tuple{T})
h (generic function with 1 method)

julia> h(::Type{T}) where {T} = hasmethod(typemin, Tuple{Type{T}})
h (generic function with 2 methods)

julia> h(pi)
true

julia> h(typeof(pi))
false

h(pi) is equivalent to the hasmethod call in the original post above, and it correctly returns true because a method exists for the call typemin(pi). But, typemin(pi) proceeds (in its implementation) to call typemin(typeof(pi)), which does not have a method, resulting in a MethodError anyway.

Furthermore, I don't think it's even theoretically possible to have a recursive hasmethod that wouldn't have any false-positives or false-negatives in general.

So this issue should probably be closed, perhaps in favor of a "recursive hasmethod and applicable" issue.

Note that applicable is very similar to hasmethod, but has an interface that is, perhaps, a bit friendlier - it takes values instead of their types:

julia> a(x) = applicable(typemin, x)
a (generic function with 1 method)

julia> a(pi)
true

julia> a(typeof(pi))
false

EDIT: for reference: hasmethod and applicable are very similar, but one difference between them (at least on the current nightly, perhaps because of #48639) is that hasmethod can be constant-folded, making it a better choice for users in some cases.

@bkamins
Copy link
Member Author

bkamins commented Aug 5, 2023

Good point. Then probably there should be a special definition for typemin and typemax for Irrational to avoid calling the generic method defined for Rational?

@LilithHafner LilithHafner removed the bug Indicates an unexpected problem or unintended behavior label Aug 7, 2023
@nalimilan
Copy link
Member

You mean we should define typemin(::typeof(pi)) = pi and typemax(::typeof(pi)) = pi? Sounds like a logical definition to me.

nsajko added a commit to nsajko/julia that referenced this issue Aug 12, 2023
@nsajko
Copy link
Contributor

nsajko commented Aug 12, 2023

I think the PR is OK, but some will be confused by one(pi) < typemin(pi) and similar.

@bkamins
Copy link
Member Author

bkamins commented Aug 12, 2023

Well one(pi) already breaks the contract of one:

julia> pi * true == pi
false

I am not sure what is best. But I meant what @nalimilan has written.

Also note that one function is not required to return the value of type passed, it only is required to guarantee to return multiplicative identity.

Note that oneunit correctly errors:

julia> oneunit(pi)
ERROR: MethodError: no method matching Irrational{:π}(::Bool)

nsajko added a commit to nsajko/julia that referenced this issue Dec 23, 2023
nsajko added a commit to nsajko/julia that referenced this issue Jan 23, 2024
nsajko added a commit to nsajko/julia that referenced this issue Jan 29, 2024
nsajko added a commit to nsajko/julia that referenced this issue Feb 1, 2024
N5N3 pushed a commit that referenced this issue Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants