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

used binary search on string indices #560

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Conversation

bjarthur
Copy link
Contributor

@bjarthur bjarthur commented Oct 25, 2023

any reason not to use the much faster searchsorted instead of findfirst for At on a dimension whose indices are String? tests pass locally and my application code is 2x faster.

@rafaqz
Copy link
Owner

rafaqz commented Oct 25, 2023

No reason I cant remember what even failed to have a type restriction there.

I think searchsorted just needs < and <= defined? Can anything be considered "ordered" without that?

Maybe we can just delete all of those types.

We can just state in the docs that Ordered lookup contents must define <. The automatic detection for it runs issorted already anyway, which must also use <.

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2023

Codecov Report

Merging #560 (64cf739) into main (b9162e1) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
+ Coverage   88.06%   88.07%   +0.01%     
==========================================
  Files          40       40              
  Lines        3326     3329       +3     
==========================================
+ Hits         2929     2932       +3     
  Misses        397      397              
Files Coverage Δ
src/LookupArrays/selector.jl 87.82% <ø> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rafaqz
Copy link
Owner

rafaqz commented Oct 26, 2023

Well that failed on Symbol... maybe we can go back to just adding String, or AbstractString and work this out more systematically later.

@rafaqz rafaqz merged commit 67394b2 into rafaqz:main Oct 26, 2023
5 checks passed
@rafaqz
Copy link
Owner

rafaqz commented Oct 26, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants