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

Deprecate countnz in favor of using count(predicate, x) #23485

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

andreasnoack
Copy link
Member

@andreasnoack andreasnoack commented Aug 28, 2017

As discussed in #23005, it is ambiguous which kind of non-zero to test with (!iszero or t -> t != 0) so with this PR the user is required to be explicit. To summarize the difference

julia> map(t -> t == 0, [rand(2) for i in 1:3])
3-element Array{Bool,1}:
 false
 false
 false

julia> map(!iszero, [rand(2) for i in 1:3])
3-element Array{Bool,1}:
 true
 true
 true

julia> map(t -> t != 0, ["Julia" for i in 1:3])
3-element Array{Bool,1}:
 true
 true
 true

julia> map(!iszero, ["Julia" for i in 1:3])
ERROR: MethodError: no method matching zero(::String)
Closest candidates are:
  zero(::Type{Base.LibGit2.GitHash}) at libgit2/oid.jl:166
  zero(::Type{Base.Pkg.Resolve.VersionWeights.VWPreBuildItem}) at pkg/resolve/versionweight.jl:80
  zero(::Type{Base.Pkg.Resolve.VersionWeights.VWPreBuild}) at pkg/resolve/versionweight.jl:122
  ...
Stacktrace:
 [1] iszero(::String) at ./number.jl:27
 [2] (::getfield(Base, Symbol("##61#62")){typeof(iszero)})(::String, ::Vararg{String,N} where N) at ./operators.jl:974
 [3] _collect at ./array.jl:678 [inlined]
 [4] collect_similar(::Array{String,1}, ::Base.Generator{Array{String,1},getfield(Base, Symbol("##61#62")){typeof(iszero)}}) at ./array.jl:625
 [5] map(::Function, ::Array{String,1}) at ./abstractarray.jl:1969

The deprecation suggests !iszero as well as t -> t != 0 but still calls t -> t != 0 to avoid breaking current behavior. We might want to revisit sparse string arrays which is the only thing I'm aware of that relies on this behavior but that is for a different PR.

@@ -576,7 +576,7 @@ end

@generated function findn(A::AbstractArray{T,N}) where {T,N}
quote
nnzA = countnz(A)
nnzA = count(!iszero, A)
Copy link
Member

Choose a reason for hiding this comment

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

This is inconsistent with the findn definition above and with its docstring, which states "determined by A[i]!=0".

6
```
"""
countnz(a) = count(x -> x != 0, a)
Copy link
Member

Choose a reason for hiding this comment

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

Note that the deprecation is different from the current definition, which (as you remarked) is technically breaking. I think the best approach would be to use Base.depwarn to print a custom deprecation warning proposing both forms, but continue calling x -> x != 0 under the hood.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the deprecation to suggest both versions following your suggestion.

count(S::SparseMatrixCSC) = count(S.nzval)
nnz(S::SparseMatrixCSC) = Int(S.colptr[S.n + 1] - 1)
count(S::SparseMatrixCSC) = count(S.nzval)
count(f, S::SparseMatrixCSC) = count(f, S.nzval) + f(zero(eltype(S)))*(prod(size(S)) - nnz(S))
Copy link
Member

Choose a reason for hiding this comment

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

What's this new method? Should go in a different commit if that's really intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

This method was missing. There is already a (dense) generic fallback here

julia/base/reduce.jl

Lines 698 to 704 in 5645fb2

function count(pred, a::AbstractArray)
n = 0
for i in eachindex(a)
@inbounds n += pred(a[i])::Bool
end
return n
end
but not a sparse version. The count(pred, x) method is suggested by the deprecation warning so I think it makes sense to add it in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Might be worth a word in the commit message.

@andreasnoack andreasnoack force-pushed the anj/countnz branch 2 times, most recently from 0e2485a to 47db98d Compare August 29, 2017 10:47
@nalimilan
Copy link
Member

Unfortunately, using x -> x != 0 in findn triggers this error: generated function body is not pure. this likely means it contains a closure or comprehension. Just create an equivalent function?

Define missing specialization of count(pred, x) for sparse arrays
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants