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

Make indexin first argument accept any iterable #23845

Closed
wants to merge 2 commits into from
Closed

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.

3 participants