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 findmin/findmax to return cartesian indices for BitMatrix #30102

Merged
merged 2 commits into from
Nov 28, 2018
Merged

Fix findmin/findmax to return cartesian indices for BitMatrix #30102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40c2d89
Move findmin and findmax BitArray methods from LinearAlgebra to Base
nalimilan Nov 20, 2018
a9161d9
Fix findmin/findmax to return cartesian indices for BitMatrix
nalimilan Nov 20, 2018
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
30 changes: 30 additions & 0 deletions base/bitarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,6 +1510,36 @@ function findprev(testf::Function, B::BitArray, start::Integer)
end
#findlast(testf::Function, B::BitArray) = findprev(testf, B, 1) ## defined in array.jl

function findmax(a::BitArray)
isempty(a) && throw(ArgumentError("BitArray must be non-empty"))
m, mi = false, 1
ti = 1
ac = a.chunks
for i = 1:length(ac)
Copy link
Member

@mbauman mbauman Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally we could just compose with findfirst(B) (and findfirst(!, B) below), but I think the current implementations would be slower than what you have here. Perhaps another place for separate improvements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. But that will require fixing findnext and findprev to accept cartesian indices (as I noted in the description).

@inbounds k = trailing_zeros(ac[i])
ti += k
k == 64 || return (true, @inbounds keys(a)[ti])
end
return m, @inbounds keys(a)[mi]
end

function findmin(a::BitArray)
isempty(a) && throw(ArgumentError("BitArray must be non-empty"))
m, mi = true, 1
ti = 1
ac = a.chunks
for i = 1:length(ac)-1
@inbounds k = trailing_ones(ac[i])
ti += k
k == 64 || return (false, @inbounds keys(a)[ti])
end
l = Base._mod64(length(a)-1) + 1
@inbounds k = trailing_ones(ac[end] & Base._msk_end(l))
ti += k
k == l || return (false, @inbounds keys(a)[ti])
return (m, @inbounds keys(a)[mi])
end

# findall helper functions
# Generic case (>2 dimensions)
function allindices!(I, B::BitArray)
Expand Down
2 changes: 1 addition & 1 deletion stdlib/LinearAlgebra/src/LinearAlgebra.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module LinearAlgebra
import Base: \, /, *, ^, +, -, ==
import Base: USE_BLAS64, abs, acos, acosh, acot, acoth, acsc, acsch, adjoint, asec, asech,
asin, asinh, atan, atanh, axes, big, broadcast, ceil, conj, convert, copy, copyto!, cos,
cosh, cot, coth, csc, csch, eltype, exp, findmax, findmin, fill!, floor, getindex, hcat,
cosh, cot, coth, csc, csch, eltype, exp, fill!, floor, getindex, hcat,
getproperty, imag, inv, isapprox, isone, iszero, IndexStyle, kron, length, log, map, ndims,
oneunit, parent, power_by_squaring, print_matrix, promote_rule, real, round, sec, sech,
setindex!, show, similar, sin, sincos, sinh, size, size_to_strides, sqrt, StridedReinterpretArray,
Expand Down
30 changes: 0 additions & 30 deletions stdlib/LinearAlgebra/src/bitarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,36 +171,6 @@ function istril(A::BitMatrix)
return true
end

function findmax(a::BitArray)
isempty(a) && throw(ArgumentError("BitArray must be non-empty"))
m, mi = false, 1
ti = 1
ac = a.chunks
for i = 1:length(ac)
@inbounds k = trailing_zeros(ac[i])
ti += k
k == 64 || return (true, ti)
end
return m, mi
end

function findmin(a::BitArray)
isempty(a) && throw(ArgumentError("BitArray must be non-empty"))
m, mi = true, 1
ti = 1
ac = a.chunks
for i = 1:length(ac)-1
@inbounds k = trailing_ones(ac[i])
ti += k
k == 64 || return (false, ti)
end
l = Base._mod64(length(a)-1) + 1
@inbounds k = trailing_ones(ac[end] & Base._msk_end(l))
ti += k
k == l || return (false, ti)
return m, mi
end

# fast 8x8 bit transpose from Henry S. Warrens's "Hacker's Delight"
# http://www.hackersdelight.org/hdcodetxt/transpose8.c.txt
function transpose8x8(x::UInt64)
Expand Down
5 changes: 5 additions & 0 deletions test/arrayops.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,11 @@ end
@test isnan(findmax([NaN, NaN, 0.0/0.0])[1])
@test findmax([NaN, NaN, 0.0/0.0])[2] == 1

# Check that cartesian indices are returned for matrices
@test argmax([10 12; 9 11]) === CartesianIndex(1, 2)
@test argmin([10 12; 9 11]) === CartesianIndex(2, 1)
@test findmax([10 12; 9 11]) === (12, CartesianIndex(1, 2))
@test findmin([10 12; 9 11]) === (9, CartesianIndex(2, 1))
end

@testset "permutedims" begin
Expand Down
2 changes: 2 additions & 0 deletions test/bitarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1523,6 +1523,8 @@ timesofar("linalg")
for b1 in [falses(v1), trues(v1),
BitArray([1,0,1,1,0]),
BitArray([0,0,1,1,0]),
BitArray([1 0; 1 1]),
BitArray([0 0; 1 1]),
bitrand(v1)]
@check_bit_operation findmin(b1)
@check_bit_operation findmax(b1)
Expand Down