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

promote fallback for division #19714

Closed
StefanKarpinski opened this issue Dec 25, 2016 · 6 comments
Closed

promote fallback for division #19714

StefanKarpinski opened this issue Dec 25, 2016 · 6 comments
Assignees
Labels
good first issue Indicates a good issue for first-time contributors to Julia help wanted Indicates that a maintainer wants help on an issue or pull request

Comments

@StefanKarpinski
Copy link
Member

Leaving this here as a breadcrumb, but if someone else wants to go for it, feel free:

https://discourse.julialang.org/t/playing-around-with-intmod/980/10

I think we should probably have a /(a::Number, b::Number) = /(promote(a,b)...) fallback. This would allow directly defining division for an integer type and promotion with e.g. Int to make 1/n to work, whereas currently it won't because the rule to convert to float first kicks in.

@StefanKarpinski StefanKarpinski added good first issue Indicates a good issue for first-time contributors to Julia help wanted Indicates that a maintainer wants help on an issue or pull request labels Dec 25, 2016
@StefanKarpinski StefanKarpinski self-assigned this Dec 25, 2016
@p-i-
Copy link

p-i- commented Dec 25, 2016

I would love to have a go at this one.

However I'm not quite following. Julia already contains that fallback: https://github.com/JuliaLang/julia/blob/master/base/promotion.jl#L193

The problem is that 1/k where k subtypes Integer hits /(x::Integer, y::Integer) = float(x)/float(y) on https://github.com/JuliaLang/julia/blob/master/base/int.jl#L35 as Integer is more specific than Number.

The following seems to work:

/{T <: Integer}(a::T, b::Integer) = /(promote(a,b)...)
/{T <: Integer}(a::Integer, b::T) = /(promote(a,b)...)

Does this look decent?

I am not confident it is robust against some awkward scenario, e.g. S and T both subtyping Integer maybe even T <: S <: Integer

@KristofferC
Copy link
Member

KristofferC commented Dec 25, 2016

The conversion to float kicks in first because that functioj is defined for two Integers which is more specific than the promote function, which is for two Numbers (and as been said, already exist).

@p-i-
Copy link

p-i- commented Dec 25, 2016

How about we simply replace /(x::Integer, y::Integer) = float(x)/float(y) on https://github.com/JuliaLang/julia/blob/master/base/int.jl#L35 with:

/{T <: Integer}(a::T, b::T) = float(a) / float(b)  # Same type
/(a::Integer, b::Integer) = /(promote(a,b)...)     # Differing types

This means if the parameters are of different types we will promote them to a common type T. If the user wishes to supply a specific overload for /(T,T) great! Otherwise we attempt to convert both to float.

Can anyone see a problem with this?

If not I'll wire it in.

@StefanKarpinski
Copy link
Member Author

The only potential problem would be if the integer promotion was potentially lossy, e.g. Int with UInt. So it might be a bit trickier than it initially seemed to get speed in the fast cases and correctness in the others.

@p-i-
Copy link

p-i- commented Dec 25, 2016

mmm...

julia> UInt64(1) / Int64(-1)
ERROR: InexactError()
 in /(::UInt64, ::Int64) at ./REPL[31]:1

julia> typeof( promote(UInt64(1), Int64(1)) )
Tuple{UInt64,UInt64}

julia> typeof( promote(UInt64(1), Int64(-1)) )
ERROR: InexactError()
 in promote(::UInt64, ::Int64) at /usr/local/Cellar/julia/HEAD-51b6b06/lib/julia/sys.dylib:?
 in promote(::UInt64, ::Int64) at /usr/local/Cellar/julia/HEAD-51b6b06/lib/julia/sys.dylib:?

So maybe {UInt64, Int64} and {Int64, UInt64} can be handled manually as special cases?

/(a::Int64, b::UInt64) = float(a) / float(b)
/(a::UInt64, b::Int64) = float(a) / float(b)

< 64 bits is no problem as e.g.

julia> typeof( promote(UInt16(1),Int16(-1)) )
Tuple{Int64,Int64}

Explicit overloads could be provided in the same way for 128-bit.

I don't feel confident to patch this through, I think it needs someone with a better perspective.

@JeffBezanson
Copy link
Member

#19645 ?

StefanKarpinski added a commit that referenced this issue Dec 29, 2016
The principle here is that if there's an implementation of a promoted
operator for some type and promotions for that type, unless other
methods are defined, the operation should always be "funnelled"
through the core operator definition for that type. The method for
`/(::Integer, ::Integer)` violated that principle since it would take
precedence in mixed-integer-type division cases even in the presence
of a same-type method definition for a custom integer type and the
appropriate promotion rules.
StefanKarpinski added a commit that referenced this issue Dec 31, 2016
The principle here is that if there's an implementation of a promoted
operator for some type and promotions for that type, unless other
methods are defined, the operation should always be "funnelled"
through the core operator definition for that type. The method for
`/(::Integer, ::Integer)` violated that principle since it would take
precedence in mixed-integer-type division cases even in the presence
of a same-type method definition for a custom integer type and the
appropriate promotion rules.
StefanKarpinski added a commit that referenced this issue Jan 3, 2017
Integer /: restrict fallback to same-type case (fix #19714)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Indicates a good issue for first-time contributors to Julia help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

No branches or pull requests

4 participants