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

Implement specialized _findin when second arg is AbstractSet #48891

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

garrison
Copy link
Member

@garrison garrison commented Mar 4, 2023

Prior to this PR, _findin converts its second argument to a Set before using it. However, this overhead is unnecessary if the argument is already an AbstractSet. _findin is in turn called by findall, so this change provides a fast path for findall(in(s), ...), where s is an AbstractSet:

julia> s = Set([1,2,3,4])
Set{Int64} with 4 elements:
  4
  2
  3
  1

julia> findall(in(s), [1,3,5,7,5,3,1])
4-element Vector{Int64}:
 1
 2
 6
 7

@garrison
Copy link
Member Author

garrison commented Jun 2, 2024

I believe that the CI failures are not related to the changes in this PR.

@adienes adienes added the performance Must go faster label Feb 21, 2025
@adienes
Copy link
Contributor

adienes commented Feb 21, 2025

seems good to me

@KristofferC
Copy link
Member

Perhaps a rebase so it gets a fresh CI run and then merge?

@adienes
Copy link
Contributor

adienes commented Feb 21, 2025

might be an exercise in frustration given that CI has like a 10% pass rate over the past few weeks

@garrison
Copy link
Member Author

Should I rebase again, or is there a better way to have the failed CI jobs run again?

@adienes
Copy link
Contributor

adienes commented Feb 24, 2025

I think close-and-reopen should trigger CI

@adienes adienes closed this Feb 24, 2025
@adienes adienes reopened this Feb 24, 2025
@adienes
Copy link
Contributor

adienes commented Feb 24, 2025

I guess not :)

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me PR is reviewed. Merge when all tests are passing performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants