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

Fix find() when called with function and non-StridedArray #8323

Merged
merged 1 commit into from
Sep 16, 2014

Conversation

nalimilan
Copy link
Member

The previous code expected testfun to be vectorized, which does not match
the behavior of other methods, nor the documentation. Especially visible
when passing a sparse matrix. Always return an Array rather than an object
similar to the input, because it makes no sense to return a sparse vector.
Remove the method matching any object so that an error is raised rather
than returning incorrect results on non-AbstractArray (e.g. Set).

This fixes the bug reported by i.pallikarakis-11 on the mailing list, namely:

A=speye(Bool,10)
find(x->x==true,A)
->0-element Array{Int64,1}

find(x->x==true,full(A))
->10-element Array{Int64,1}:
   1
  12
  23
  34
  45
  56
  67
  78
  89
 100

I'm not completely sure about the AbstractArray requirement, but it doesn't make sense to me to return indexes for non-arrays. The previous code would happily work on Sets, but would return nonsensical results.

(BTW, I'm aware that calling find(testf, x) with x a sparse matrix is not very efficient, but at least it must not give wrong results.)

The previous code expected testfun to be vectorized, which does not match
the behavior of other methods, nor the documentation. Especially visible
when passing a sparse matrix. Always return an Array rather than an object
similar to the input, because it makes no sense to return a sparse vector.
Remove the method matching any object so that an error is raised rather
than returning incorrect results on non-AbstractArray (e.g. Set).
@nalimilan
Copy link
Member Author

I realize this change has the drawback that scalars are only supported if they are Numbers:

julia> find(s -> ismatch(r"a", s), ["az"])
1-element Array{Int64,1}:
 1

julia> find(s -> ismatch(r"a", s), "az")
ERROR: `find` has no method matching find(::Function, ::ASCIIString)

But if find works on any object, not only numbers, then it gives misleading results for non-scalars like Set.

@ivarne
Copy link
Member

ivarne commented Sep 12, 2014

See also my answer to the other post about the problem.

This looks like a bug to me, and it was caused by c330b1d and it links to #987.

The bug is silent because there already was a generic function that could find things in non iterable collections introduced in b8f47e8 (that seems like a stupid use of find and it should probably be restricted to Number).

@@ -1082,7 +1082,7 @@ function find(A::StridedArray)
end

find(x::Number) = x == 0 ? Array(Int,0) : [1]
find(testf::Function, x) = find(testf(x))
find(testf::Function, x::Number) = testf(x) == 0 ? Array(Int,0) : [1]
Copy link
Member

Choose a reason for hiding this comment

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

Should be !testf(x) instead of testf(x)==0, but false == 0 so this works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, not pretty. :-/ I could have fixed that. Worth another commit?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but a commit is cheap: 6b3276f

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but for me it implies bothering somebody with a pull request.

While we're tweaking style, how about writing it testf(x) ? [1] : Array(Int,0) instead? :-p

Copy link
Member

Choose a reason for hiding this comment

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

Or

testf(x) ? Int[1] : Int[]

Which will also make the function type stable. (Sorry! Array constructors confuse me)

I don't think Pull requests is bothering for small changes like this, that are obviously correct. A unrelated discussion that goes on and on, is a different matter tough.

Copy link
Member

Choose a reason for hiding this comment

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

Or testf(x) ? [1] : Int[] ?!

JeffBezanson added a commit that referenced this pull request Sep 16, 2014
Fix find() when called with function and non-StridedArray
@JeffBezanson JeffBezanson merged commit 79ce765 into JuliaLang:master Sep 16, 2014
@JeffBezanson
Copy link
Member

cc @JuliaBackports

ivarne pushed a commit that referenced this pull request Sep 16, 2014
The previous code expected testfun to be vectorized, which does not match
the behavior of other methods, nor the documentation. Especially visible
when passing a sparse matrix. Always return an Array rather than an object
similar to the input, because it makes no sense to return a sparse vector.
Remove the method matching any object so that an error is raised rather
than returning incorrect results on non-AbstractArray (e.g. Set).

Backport of: ae641c9
Issue: #8323
@ivarne
Copy link
Member

ivarne commented Sep 16, 2014

Backported in f0214c3

ivarne added a commit that referenced this pull request Sep 17, 2014
@nalimilan nalimilan deleted the find branch September 17, 2014 10:30
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.

4 participants