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.

Sign up for GitHub

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

Fix behavior of foldr on empty arrays (#20144) #20160

Merged
merged 6 commits into from
Jan 27, 2017
Merged

Fix behavior of foldr on empty arrays (#20144) #20160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
75ae5fb
Fix foldr behavior
victhorio Jan 20, 2017
2a06f9c
Added tests
victhorio Jan 20, 2017
73bfee7
Remove unecessary semicolon
victhorio Jan 20, 2017
222810b
Reference relevant issues on tests
victhorio Jan 20, 2017
d3276ee
Better check for empty arrays on mapfoldr
victhorio Jan 22, 2017
5abf3b8
More tests for fold/reduce with type equality check
victhorio Jan 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions base/reduce.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ foldl(op, itr) = mapfoldl(identity, op, itr)
function mapfoldr_impl(f, op, v0, itr, i::Integer)
# Unroll the while loop once; if v0 is known, the call to op may
# be evaluated at compile time
if i == 0
if isempty(itr)
return r_promote(op, v0)
else
x = itr[i]
Expand All @@ -131,7 +131,13 @@ mapfoldr(f, op, v0, itr) = mapfoldr_impl(f, op, v0, itr, endof(itr))
Like `mapfoldr(f, op, v0, itr)`, but using the first element of `itr` as `v0`. In general,
this cannot be used with empty collections (see `reduce(op, itr)`).
"""
mapfoldr(f, op, itr) = (i = endof(itr); mapfoldr_impl(f, op, f(itr[i]), itr, i-1))
function mapfoldr(f, op, itr)
i = endof(itr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the endof could be moved to after the if now, but I don't know if there are any collections where endof would be expensive enough for it to matter when isempty is true

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense though, the only reason it's before is because I was previously using the i value in the if. Should I make the change?

Copy link
Contributor

@tkelman tkelman Jan 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably not worth waiting in the CI queue for again, if nobody else has any more substantial feedback. Maybe next time someone's working around this bit of code.

Copy link
Contributor

@pabloferz pabloferz Jan 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is that you could leave the i = endof(itr) and use if done(itr, i) instead of if isempty(itr). Other than that, I don't have more comments.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pabloferz I considered that option over isempty but I found out that because of the following behavior I couldn't use it

julia> itr = Float64[]
0-element Array{Float64,1}

julia> i = endof(itr)
0

julia> done(itr, i)
false

It would only return true when i == 1 for the empty array.

Copy link
Contributor

@pabloferz pabloferz Jan 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's right (forgot that this recurses backwards). Then, it is fine as is right now.

if isempty(itr)
return Base.mr_empty_iter(f, op, itr, iteratoreltype(itr))
end
return mapfoldr_impl(f, op, f(itr[i]), itr, i-1)
end

"""
foldr(op, v0, itr)
Expand Down
6 changes: 6 additions & 0 deletions test/reduce.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# This file is a part of Julia. License is MIT: http://julialang.org/license

# fold(l|r) & mapfold(l|r)
@test foldl(+, Int64[]) === Int64(0) # In reference to issues #7465/#20144 (PR #20160)
@test foldl(+, Int16[]) === Int32(0)
@test foldl(-, 1:5) == -13
@test foldl(-, 10, 1:5) == -5

Expand All @@ -16,13 +18,17 @@
@test Base.mapfoldl((x)-> x ⊻ true, |, [true false true false false]) == true
@test Base.mapfoldl((x)-> x ⊻ true, |, false, [true false true false false]) == true

@test foldr(+, Int64[]) === Int64(0) # In reference to issue #20144 (PR #20160)
@test foldr(+, Int16[]) === Int32(0)
@test foldr(-, 1:5) == 3
@test foldr(-, 10, 1:5) == -7

@test Base.mapfoldr(abs2, -, 2:5) == -14
@test Base.mapfoldr(abs2, -, 10, 2:5) == -4

# reduce & mapreduce
@test reduce(+, Int64[]) === Int64(0) # In reference to issue #20144 (PR #20160)
@test reduce(+, Int16[]) === Int32(0)
@test reduce((x,y)->"($x+$y)", 9:11) == "((9+10)+11)"
@test reduce(max, [8 6 7 5 3 0 9]) == 9
@test reduce(+, 1000, 1:5) == (1000 + 1 + 2 + 3 + 4 + 5)
Expand Down