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

Move internal searchsorted* methods to _searchsorted* #51177

Closed
wants to merge 1 commit into from

Conversation

LilithHafner
Copy link
Member

Technically, this is not a breaking change. Practically, we'll see. We might need to add in some new deprecated methods to ease the transition, or if folks have legitimate uses that warrant expanding the API to the 5-arg version we could add a new (safe) 5-arg version. See #51176 for motivation.

@LilithHafner LilithHafner added the needs pkgeval Tests for all registered packages should be run with this change label Sep 4, 2023
@LilithHafner
Copy link
Member Author

Too breaking

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 5, 2023

Looks like a SparseArrays issue, since callers should be using view rather than this internal API. We can perhaps throw a boundscheck call in these though without much cost, and then defer to the _searchsorted version.

@LilithHafner
Copy link
Member Author

I falsely assumed that views would have a performance overhead

julia> x = cumsum(rand(1000));

julia> @btime searchsortedfirst($x, 250, 400, 600, Base.Forward)
  14.612 ns (0 allocations: 0 bytes)
510

julia> @btime searchsortedfirst(view($x, 400:600), 250)
  14.612 ns (0 allocations: 0 bytes)
111

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 5, 2023

They should imply a boundscheck, I think, so not quite free. But we fixed the performance issue with using them here a couple Julia releases ago.

@giordano giordano deleted the searchsorted-api-cleanup branch February 25, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants