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

avoid calling zero in reduce when possible #21097

Open
dlfivefifty opened this issue Mar 19, 2017 · 10 comments · May be fixed by #55318
Open

avoid calling zero in reduce when possible #21097

dlfivefifty opened this issue Mar 19, 2017 · 10 comments · May be fixed by #55318
Labels
fold sum, maximum, reduce, foldl, etc.

Comments

@dlfivefifty
Copy link
Contributor

See discussion https://discourse.julialang.org/t/interface-for-number/2723/5

I recently ran into an issue overriding zero for my own Number:

julia> immutable Infinity <: Number end

julia> Base.zero(::Infinity) = 0

julia> import Base: +

julia> +(::Infinity,::Int) = Infinity()
+ (generic function with 181 methods)

julia> reduce(+,[Infinity(),Infinity()])
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Infinity
This may have arisen from a call to the constructor Infinity(...),
since type constructors fall back to convert methods.
Stacktrace:
 [1] _mapreduce(::Base.#identity, ::Base.#+, ::IndexLinear, ::Array{Infinity,1}) at ./reduce.jl:260
 [2] reduce(::Function, ::Array{Infinity,1}) at ./reduce.jl:321

The issue appears to be that zero{T<:Number}(::T) needs is expected to be convertible to T.

Is this a bug? Otherwise, this should be documented somewhere (e.g., a description of the interface for Number)

@JeffBezanson
Copy link
Member

I think you need to define zero(::Type{Infinity}). But there are certainly many cases where we expect to be able to convert numbers to other number types.

@dlfivefifty
Copy link
Contributor Author

I think you need to define zero(::Type{Infinity})

That doesn't fix the issue. In any case, what is expected should be documented.

It's not clear to me what a Number means: not all sets of numbers include zero...another example would be numbers on the unit circle |z| = 1.

@JeffBezanson
Copy link
Member

Ah, this case (reduce) seems to be one that just uses convert, and not always zero. I think it's deliberately unclear what Number means; for example should we require typeof(zero(T)) === T? I don't know.

After looking into it a bit more, I'd summarize this as a quasi-bug in reduce, in that it doesn't support types that can't be converted to the type of zero(T). It would be nice if we could maintain its functionality with fewer assumptions.

@dlfivefifty
Copy link
Contributor Author

dlfivefifty commented Mar 20, 2017

it's deliberately unclear what Number means

Hopefully this will be changed before 1.0. It would be good to have a Number interface outlined.

@nalimilan
Copy link
Member

What looks like a bug to me is that reduce always tries to compute the zero element even when the collection is not empty. It seems like the error should happen only when the zero is actually needed. (Though I note the docstring says "It is unspecified whether v0 is used for non-empty collections.")

@timholy
Copy link
Member

timholy commented Mar 20, 2017

Now that one(T) does not return an object of type T, zero has become the new workhorse for "give me an object of type T." I think we need to require that...or that @dlfivefifty needs to do the work to figure out the larger implications. @dlfivefifty, can you audit the code in base/ and figure out how many uses there are of zero that implicitly assume it returns an object of type T?

@nalimilan
Copy link
Member

Note that in the present case the issue isn't that the zero isn't of type T, it's that it cannot be converted to T at all.

@dlfivefifty
Copy link
Contributor Author

I think we need to require that...or that @dlfivefifty needs to do the work to figure out the larger implications

I'm fine with that being a requirement, it just needs to be documented

@stevengj
Copy link
Member

For non-empty collections, it would be good if reduce(+, x) didn't call zero at all. (This function has been notoriously difficult to get right in a type-stable way.)

@JeffBezanson JeffBezanson changed the title Return type ofzero{T<:Number}(::T) must now be promotable to T avoid calling zero in reduce when possible Mar 22, 2017
@simonbyrne
Copy link
Contributor

This now works for plain reduce:

julia> struct Infinity <: Number end

julia> Base.:+(::Infinity,::Infinity) = Infinity()

julia> reduce(+,[Infinity(),Infinity()])
Infinity()

julia> zero(Infinity())
ERROR: MethodError: no method matching Infinity(::Int64)

But not for reduce with dims:

julia> reduce(+,[Infinity(),Infinity()], dims=1)
ERROR: MethodError: no method matching Infinity(::Int64)

@brenhinkeller brenhinkeller added the fold sum, maximum, reduce, foldl, etc. label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fold sum, maximum, reduce, foldl, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants