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

vsartor · 2017-01-20T19:19:55Z

This is a fix for #20144.

When fixing #7465, a check was added based on mr_empty to mapfoldl, which is called by both foldl and reduce, so they would have better behavior with empty arrays.

This implements an equivalent check on foldr so that it will have the same behavior as foldl and reduce.

I also added tests to make sure "folding" an operator such as + on an empty array gives the expected result.

This is my first PR, so sorry for any mistakes.

When foldr is called with two arguments and an empty array, mapfoldr does not make a check for empty array and tries to access the first element. This check is present in mapfoldl. The commit adds equivalent behavior to mapfoldl.

Sacha0 · 2017-01-20T19:22:30Z

base/reduce.jl

@@ -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);


No need for the trailing semicolon; remove?

Forgot to remove it, sorry.
Removed.

Sacha0 · 2017-01-20T19:37:34Z

test/reduce.jl

@@ -16,6 +17,7 @@
 @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[]) == 0


Perhaps add a comment to each of these tests referencing the relevant issue and pull request?

In the foldr test I can just reference issue #20144 and PR #20160 (this one), but on the foldl test I can only reference issue #7465 since there is no PR.
@Sacha0, do I just reference the relevant issue for the foldl test or do I add the SHA of the relevant commit? (b674eed)

Referencing via the issue / pull request numbers (e.g. #20144, #20160) would be great. Given #20160 adds the foldl test and #20144/#20160 contain the relevant discussion, perhaps reference #20144 and #20160 for that test as well, rather than (or in addition to) #7465? Thanks!

Gotcha. Done.
Thanks for reviewing!

Sacha0 · 2017-01-20T19:39:18Z

Welcome to Julia @Vsart! Much thanks for this great first pull request!

pabloferz · 2017-01-20T23:20:25Z

base/reduce.jl

-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)
+ if i == 0


I belive you don't need the i = endof(itr) here, instead, you can do if isempty(itr).

@pabloferz, the existing implementation uses i in the call to mapfoldr_impl (see just below)?

Oh right, missed that. Carry one then.

is endof(itr) == 0 correct for all iterator types though?

I wondered the same given the mapfoldl implementation takes what might be a more general approach. Thoughts?

Yeah, start and done is definitively better and general enough (the endof(itr) == 0 is actually what threw me out)

I copied the check from mapfoldr_impl (here) which tests if the i argument is 0, I'm assuming that if the one I implemented in mapfoldr should change so should that check?

vsartor · 2017-01-22T03:57:57Z

The build failure on OSX from Travis seems to be unrelated.

Sacha0 · 2017-01-22T18:20:39Z

@nanosoldier runbenchmarks(ALL, vs=":master")

tkelman · 2017-01-22T19:39:48Z

I'd maybe make some of the reduce tests more specific and also test for type equality via ===, but you'd need to use Int for the element type rather than Int64 (or compare to an Int64 rhs instead of an Int)

vsartor · 2017-01-22T20:25:20Z

@tkelman About the type equality tests, do you mean testing for example

foldr(+, Int16[]) === Int16(0)

or you meant something else?

Currently this is not true (for both the new foldr and the old foldl) Some examples below

julia> typeof(foldl(+, Int8[]))
Int32

julia> typeof(foldl(+, Int16[]))
Int32

julia> typeof(foldl(+, Int32[]))
Int64

julia> typeof(foldl(+, Int64[]))
Int64

Note that this behavior is not particular to empty arrays.

nanosoldier · 2017-01-22T22:25:12Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

tkelman · 2017-01-22T22:32:37Z

Right, reductions currently widen mostly to make them less susceptible to overflow. But where we or packages care at all about the return type, it's worth encoding that specificity into the tests. If we ever intend to change the return type, we'll have to visibly change the tests. What testing the types would avoid is accidentally changing the return types and not noticing.

StefanKarpinski · 2017-01-27T14:54:27Z

This looks good to me. Can all the reviewer please approve the changes?

tkelman · 2017-01-27T15:03:16Z

base/reduce.jl

@@ -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)


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

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?

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.

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.

@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.

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

tkelman

should be squashed on merge, lgtm

Sacha0 · 2017-01-27T19:43:15Z

Thanks @Vsart!

vsartor added 2 commits January 20, 2017 15:55

Fix foldr behavior

75ae5fb

When foldr is called with two arguments and an empty array, mapfoldr does not make a check for empty array and tries to access the first element. This check is present in mapfoldl. The commit adds equivalent behavior to mapfoldl.

Added tests

2a06f9c

Sacha0 reviewed Jan 20, 2017

View reviewed changes

Remove unecessary semicolon

73bfee7

Sacha0 reviewed Jan 20, 2017

View reviewed changes

Reference relevant issues on tests

222810b

pabloferz reviewed Jan 20, 2017

View reviewed changes

vsartor changed the title ~~RFC: Fix behavior of foldr on empty arrays (#20144)~~ Fix behavior of foldr on empty arrays (#20144) Jan 21, 2017

Better check for empty arrays on mapfoldr

d3276ee

More tests for fold/reduce with type equality check

5abf3b8

ararslan added the domain:collections Data structures holding multiple items, e.g. sets label Jan 23, 2017

StefanKarpinski approved these changes Jan 27, 2017

View reviewed changes

tkelman reviewed Jan 27, 2017

View reviewed changes

tkelman approved these changes Jan 27, 2017

View reviewed changes

pabloferz approved these changes Jan 27, 2017

View reviewed changes

Sacha0 approved these changes Jan 27, 2017

View reviewed changes

Sacha0 merged commit e63fec8 into JuliaLang:master Jan 27, 2017

vsartor mentioned this pull request Jan 28, 2017

reduce and foldr/foldl are inconsistent for + and empty vectors #20144

Closed

ararslan mentioned this pull request Apr 22, 2017

foldr failing for one element arrays on 0.6 #21493

Closed

KristofferC mentioned this pull request May 26, 2017

add test for foldr on one element range #22085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vsartor commented Jan 20, 2017

Sacha0 Jan 20, 2017

vsartor Jan 20, 2017

Sacha0 Jan 20, 2017

vsartor Jan 20, 2017

Sacha0 Jan 20, 2017

vsartor Jan 20, 2017

Sacha0 commented Jan 20, 2017

pabloferz Jan 20, 2017

Sacha0 Jan 20, 2017

pabloferz Jan 20, 2017

tkelman Jan 21, 2017

Sacha0 Jan 21, 2017

pabloferz Jan 21, 2017

vsartor Jan 21, 2017 •

edited

Loading

vsartor commented Jan 22, 2017

Sacha0 commented Jan 22, 2017

tkelman commented Jan 22, 2017

vsartor commented Jan 22, 2017 •

edited

Loading

nanosoldier commented Jan 22, 2017

tkelman commented Jan 22, 2017

StefanKarpinski commented Jan 27, 2017

tkelman Jan 27, 2017

vsartor Jan 27, 2017

tkelman Jan 27, 2017 •

edited

Loading

pabloferz Jan 27, 2017 •

edited

Loading

vsartor Jan 27, 2017

pabloferz Jan 27, 2017 •

edited

Loading

tkelman left a comment

Sacha0 commented Jan 27, 2017

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

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

Conversation

vsartor commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsartor Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

vsartor commented Jan 22, 2017

Sacha0 commented Jan 22, 2017

tkelman commented Jan 22, 2017

vsartor commented Jan 22, 2017 • edited Loading

nanosoldier commented Jan 22, 2017

tkelman commented Jan 22, 2017

StefanKarpinski commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

pabloferz Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

tkelman left a comment

Choose a reason for hiding this comment

Sacha0 commented Jan 27, 2017

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

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

vsartor Jan 21, 2017 •

edited

Loading

vsartor commented Jan 22, 2017 •

edited

Loading

tkelman Jan 27, 2017 •

edited

Loading

pabloferz Jan 27, 2017 •

edited

Loading

pabloferz Jan 27, 2017 •

edited

Loading