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(in(b), a) to return cartesian indices for matrix a #30226

Merged
merged 3 commits into from Dec 7, 2018

Conversation

6 participants
@nalimilan
Copy link
Contributor

commented Dec 1, 2018

For consistency with find(x -> in(b), a).
Restrict function signatures for clarity, as a needs to support keys/pairs and these internal functions are only called on arrays and tuples.

Similar to #30102.

Fix find(in(b), a) to return cartesian indices for matrix a
For consistency with find(x -> in(b), a).
Restrict function signatures for clarity, as a needs to support keys/pairs and
these internal functions are only called on arrays and tuples.
@garrison

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Thanks @nalimilan. Here is the example from Slack that led to this:

julia> findall(in([5, 6]), [5 6; 7 8])
2-element Array{Int64,1}:
 1
 3

julia> findall(!in([5, 6]), [5 6; 7 8])
2-element Array{CartesianIndex{2},1}:
 CartesianIndex(2, 1)
 CartesianIndex(2, 2)

julia> findall(x -> x in [5, 6], [5 6; 7 8])
2-element Array{CartesianIndex{2},1}:
 CartesianIndex(1, 1)
 CartesianIndex(1, 2)
@garrison

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

CC @mbauman

Also: I agree that this is a minor change, but I do want to note that it is technically breaking AFAICT.

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2018

That is pretty much what the "minor change" label means.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

“Minor change” is a non-scary name for “technically breaking but unlikely to cause anyone real problems, which we will test by running PkgEval before releasing”.

Add NEWS.md entry
[ci skip]
@mbauman

mbauman approved these changes Dec 1, 2018

@mbauman

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

I just fixed the conflict — let's merge this before the feature freeze for 1.1.

@mbauman mbauman added this to the 1.1 milestone Dec 6, 2018

@JeffBezanson JeffBezanson merged commit cd7041e into master Dec 7, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@JeffBezanson JeffBezanson deleted the nl/findin branch Dec 7, 2018

garrison added a commit to garrison/UniqueVectors.jl that referenced this pull request Dec 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.