Bug in function `all`

[zdlspace0528]

I find a bug about all.

here is the result.

julia> all([3, 3, 3], dims = 1)
1-element Vector{Bool}:
 1

The result maybe wrong.

julia> for i in 1:10
           @show all(i * ones(Int, 3), dims = 1)
       end
all(i * ones(Int, 3), dims = 1) = Bool[1]
all(i * ones(Int, 3), dims = 1) = Bool[0]
all(i * ones(Int, 3), dims = 1) = Bool[1]
all(i * ones(Int, 3), dims = 1) = Bool[0]
all(i * ones(Int, 3), dims = 1) = Bool[1]
all(i * ones(Int, 3), dims = 1) = Bool[0]
all(i * ones(Int, 3), dims = 1) = Bool[1]
all(i * ones(Int, 3), dims = 1) = Bool[0]
all(i * ones(Int, 3), dims = 1) = Bool[1]
all(i * ones(Int, 3), dims = 1) = Bool[0]

[zdlspace0528]

We talk about this problem here

[LilithHafner]

any throws an error:

julia> any([3, 3, 3], dims = 1)
ERROR: InexactError: Bool(3)
Stacktrace:
  [1] Bool
    @ ./float.jl:158 [inlined]
  [2] convert
    @ ./number.jl:7 [inlined]
  [3] setindex!
    @ ./array.jl:959 [inlined]
  [4] setindex!
    @ ./multidimensional.jl:674 [inlined]
  [5] _mapreducedim!(f::typeof(identity), op::typeof(|), R::Vector{Bool}, A::Vector{Int64})
    @ Base ./reducedim.jl:311
  [6] mapreducedim!(f::Function, op::Function, R::Vector{Bool}, A::Vector{Int64})
    @ Base ./reducedim.jl:324
  [7] _mapreduce_dim(f::Function, op::Function, #unused#::Base._InitialValue, A::Vector{Int64}, dims::Int64)
    @ Base ./reducedim.jl:371
  [8] #mapreduce#758
    @ ./reducedim.jl:357 [inlined]
  [9] #_any#812
    @ ./reducedim.jl:1023 [inlined]
 [10] _any
    @ ./reducedim.jl:1023 [inlined]
 [11] #_any#811
    @ ./reducedim.jl:1022 [inlined]
 [12] _any
    @ ./reducedim.jl:1022 [inlined]
 [13] #any#785
    @ ./reducedim.jl:1003 [inlined]
 [14] top-level scope
    @ REPL[428]:1

[LilithHafner]

The error we want is ERROR: TypeError: non-boolean (Int64) used in boolean context

[LilithHafner]

From @pfitzseb in the linked discussion

This boils down to

julia> mapreduce(identity, &, [3,3,3]; dims=1)
1-element Vector{Bool}:
1

julia> mapreduce(identity, &, [2,2,2]; dims=1)
1-element Vector{Bool}:
0

and definitely is a bug.

The problem is that we are using & and | for all and any. Unfortunately, || and && are not functions.

[LilithHafner]

We should be using something that errors on non-booleans. We can write up custom functions and use these instead:

boolean_and(a::Bool, b::Bool) = a & b
boolean_and(a::Bool, b) = throw(TypeError(:boolean_and, Bool, b))
boolean_and(a, b) = throw(TypeError(:boolean_and, Bool, a))
boolean_or(a::Bool, b::Bool) = a | b
boolean_or(a::Bool, b) = throw(TypeError(:boolean_or, Bool, b))
boolean_or(a, b) = throw(TypeError(:boolean_or, Bool, a))

[GunnarFarneback]

Notice that the investigation revealed two separate bugs with reductions over an axis.

1. This should error like its non-axis version:

julia> all([3, 3, 3], dims = 1)
1-element Vector{Bool}:
 1

julia> all([3, 3, 3])
ERROR: TypeError: non-boolean (Int64) used in boolean context

2. The downstream mapreduce also gives an incorrect result along an axis:

julia> mapreduce(identity, &, [3,3,3], dims = 1)
1-element Vector{Bool}:
 1

julia> mapreduce(identity, &, [3,3,3])
3

[pfitzseb]

The reducedim part of this bug is caused by

julia/base/reducedim.jl

Lines 206 to 207 in bd8dbc3

	reducedim_init(f, op::typeof(&), A::AbstractArrayOrBroadcasted, region) = reducedim_initarray(A, region, true)
	reducedim_init(f, op::typeof(|), A::AbstractArrayOrBroadcasted, region) = reducedim_initarray(A, region, false)

which causes the returned array to be unconditionally initialized as a Array{Bool}, even though &/| aren't guaranteed to return booleans.

[LilithHafner]

@GunnarFarneback, the problem you labeled 2 is at the intersection of this issue and #45566.
The linked PR fixes 1 and turns 2 from a wrong answer into an error.

[GunnarFarneback]

I assume you mean the opposite. It would be good if it added tests for both problems.

[LilithHafner]

The expected result in 1 is an error, so by "fixes 1" I also mean "turn a wrong answer into an error", Another way I could have said it that might be more clear is "the linked PR turns both these cases into an error. However, 2 should not error. That should be fixed elsewhere".

I added tests for 1, the issue in the OP. Because I did't solve 2, I also didn't add tests for it because those tests would be broken.

[GunnarFarneback]

Thanks, I misunderstood. Turning a wrong answer into an error sounds a bit scary, even if both are incorrect.

[StefanKarpinski]

Unless I'm missing something, the middle method of each of these is unnecessary. You can just do:

boolean_and(a::Bool, b::Bool) = a & b
boolean_or(a::Bool, b::Bool) = a | b
boolean_and(a, b) = throw(TypeError(:boolean_and, Bool, a))
boolean_or(a, b) = throw(TypeError(:boolean_or, Bool, a))

Could generalize this to

function boolean_op(f::Function)
    op(a, b) = throw(TypeError(nameof(f), Bool, a))
    op(a::Bool, b::Bool) = f(a, b)
end

Although that does produce annoyingly ugly stack traces on account of the inner function.

[pfitzseb]

This is unrelated to correctness, but shouldn't all/any short-circuit? There really is no need to check the whole array after all (although you'll of course end up with a sort of ragged iteration scheme which could be slower in some cases, I guess).