Skip to content

Conversation

@garrison
Copy link
Member

@garrison garrison commented Sep 23, 2017

I also fixed the docstring and added a test where a matrix is passed.

Also compactified the tests according to https://github.com/JuliaLang/julia/pull/8622/files#r18615289 :-)

@garrison
Copy link
Member Author

Actually, with this change, the first argument to indexin can now be a scalar as well:

julia> indexin(4, [3,4,5])
0-dimensional Array{Int64,0}:
2

but notice that it returns it in the form of a zero-dimensional array. Should this call return an Int instead?

@StefanKarpinski
Copy link
Member

Should this call return an Int instead?

Yes, 100%.

@garrison
Copy link
Member Author

OK, I split this into two commits: the first updates the docstring and tests according to the old functionality, while the second adds new functionality.

I'm actually unsure the best way to implement the goal of having a scalar return a scalar instead of a 0D array. My current approach has one broken test (marked by @test_broken) that I would claim ought to work if this were implemented correctly. Also, the docstring is now inaccurate since the method no longer always returns an array.

@garrison garrison changed the title Make indexin first argument accept any iterable WIP: Make indexin first argument accept any iterable Sep 27, 2017
@garrison
Copy link
Member Author

I fixed the 0-dimensional array issue by changing from a comprehension to calling map instead. I also decided that IHMO, having the docstring refer to an array is appropriate since the most common use involves returning an array, not a scalar.

One remaining issue is that indexin(Dict(3=>4, 5=>6), [1,2,3,4]) currently throws a BoundsError because Dict cannot be indexed by "positions" 1 and 2. But this actually calls a deprecated method (map(f, d::T) where T <: Associative), so it will instead throw MethodError in the future, which is more appropriate. At the moment I'm trying to determine if there may be other types that would fail in less graceful ways.

@garrison garrison changed the title WIP: Make indexin first argument accept any iterable Make indexin first argument accept any iterable Sep 29, 2017
@garrison
Copy link
Member Author

garrison commented Oct 7, 2017

I'm pretty happy with how this stands -- I don't think the Dict issue I mentioned above is a big deal. If desired, I could always check that eachindex(b) == 1:length(b) (and throw an ArgumentError if it does not).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

search & find The find* family of functions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants