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

Some corrections to findn #25365

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Conversation

fredrikekre
Copy link
Member

Fixes this oversight from #23812

julia> findn(rand(2))
┌ Warning: In the future `find(A)` will only work on boolean collections. Use `find(x->x!=0, A)` instead.
│   caller = findn(::Array{Float64,1}) at array.jl:1803
└ @ Base array.jl:1803
2-element Array{Int64,1}:
 1
 2

and adjust the docs for findn (fix #25343).

Two other things I noticed:

  • It is a bit incosistent that findn return a tuple for all cases except for vectors. Should we also apply
diff --git a/base/array.jl b/base/array.jl
index d482e18..e1a8ce4 100644
--- a/base/array.jl
+++ b/base/array.jl
@@ -1800,7 +1800,7 @@ end
 find(x::Bool) = x ? [1] : Vector{Int}()
 find(testf::Function, x::Number) = !testf(x) ? Vector{Int}() : [1]
 
-findn(A::AbstractVector) = find(x -> x != 0, A)
+findn(A::AbstractVector) = (find(x -> x != 0, A), )
 
 """
     findn(A)
  • Are the methods here in base/array.jl for vectors and matrices needed, or should we simply remove them in favor of
    @generated function findn(A::AbstractArray{T,N}) where {T,N}
    quote
    nnzA = count(_countnz, A)
    @nexprs $N d->(I_d = Vector{Int}(uninitialized, nnzA))
    k = 1
    @nloops $N i A begin
    @inbounds if (@nref $N A i) != zero(T)
    @nexprs $N d->(I_d[k] = i_d)
    k += 1
    end
    end
    @ntuple $N I
    end
    end
    (which for some simple test cases seems to be more efficient.)

@andreasnoack
Copy link
Member

I think the special cases for Vector and Matrix should be deleted.

@fredrikekre
Copy link
Member Author

fredrikekre commented Jan 3, 2018

I think the special cases for Vector and Matrix should be deleted.

Alright, that will also have the result that findn(::AbstractVector) return a 1-tuple, so takes care of both points then I suppose.

adjust documentation for findn (fix JuliaLang#25343)
remove special special cases for findn(::AbstractArray{T,(1|2)})
@nalimilan
Copy link
Member

nalimilan commented Jan 3, 2018

I'm fine with the proposed changes, but these functions should probably be removed (see #24910). Maybe hold off the PR until we have made a decision about it?

@andreasnoack
Copy link
Member

Aren't these fixes separate from the renaming decision? I don't see how merging this would interfere with the renaming/removal.

@nalimilan
Copy link
Member

No, it wouldn't interfere, but it's not really useful to change a function just before deprecating it? Not a big deal either way.

@andreasnoack andreasnoack merged commit 3e4ab51 into JuliaLang:master Jan 4, 2018
@fredrikekre fredrikekre deleted the fe/findn branch January 4, 2018 12:13
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.

slight inaccuracy in findn docstring
3 participants