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

nalimilan
Copy link
Member

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.

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.
@nalimilan nalimilan added bugfix This change fixes an existing bug search & find The find* family of functions minor change Marginal behavior change acceptable for a minor release labels Dec 1, 2018
@garrison
Copy link
Sponsor Member

garrison 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
Copy link
Sponsor Member

garrison 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
Copy link
Sponsor Member

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

@StefanKarpinski
Copy link
Sponsor Member

“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”.

@mbauman
Copy link
Sponsor Member

mbauman 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
@JeffBezanson JeffBezanson deleted the nl/findin branch December 7, 2018 06:27
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
Labels
bugfix This change fixes an existing bug minor change Marginal behavior change acceptable for a minor release search & find The find* family of functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants