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

SearchBox: Change to the Dice-Sørensen coefficient #605

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

Dlurak
Copy link
Collaborator

@Dlurak Dlurak commented Sep 29, 2024

Description

  • Change to Dice-Sørensen coefficient for the searchbox sorting on stars and presets
  • this algorithm is way slower then the old match(str1, str2).length way of doing things. So I significantly optimized getPresetOptions function
  • The new dependency is of minimal size, very fast and overall a good way to use the dice coefficient in javascript. It even includes types.

Copy link

vercel bot commented Sep 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
osmapp ✅ Ready (Inspect) Visit Preview Oct 8, 2024 9:34am

@zbycz
Copy link
Owner

zbycz commented Oct 7, 2024

Looks ok. Thanks for the reasoning behind the dependency.

It looks working, though I don't see the enhancement much. Does it basically works like before? Could you perhaps supply some test stars JSON and queries which you think are better?

btw, One thing I would expecially like is to match the whole substring with bigger rank boost. Eg. for "lom" it is first, but "lom k" should be even more prominent, but is no. Also I know this was there before.

lom lom k
image image

@Dlurak
Copy link
Collaborator Author

Dlurak commented Oct 7, 2024

The dice coefficient is more resistant to small typos and works great with multiple words. I don't know how to resolve your issue

@zbycz
Copy link
Owner

zbycz commented Oct 7, 2024

The dice coefficient is more resistant to small typos and works great with multiple words. I don't know how to resolve your issue

Ok, lets' leave it for later 👌

@Dlurak
Copy link
Collaborator Author

Dlurak commented Oct 7, 2024

So this pr is fine for merging?

Copy link
Owner

@zbycz zbycz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, lets test it out in prod.

@Dlurak Dlurak enabled auto-merge (squash) October 8, 2024 09:30
@Dlurak Dlurak merged commit 600cd3f into zbycz:master Oct 8, 2024
2 checks passed
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.

2 participants