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

Fixes #15, #17, #18, and does other improvements #16

Merged
merged 16 commits into from
Jan 7, 2021

Conversation

luistrivelatto
Copy link
Contributor

Added tests reproducing the crash in #15, and fixed by removing empty tokens after tokenizing.

@luistrivelatto
Copy link
Contributor Author

Added some commits that also fixes #17, #18, and fixes an apparent bug in weighted search.

Previously, single-weighted searches returned score = 0, due to the single weight zeroing them out. Also, comparing the results of searching with a single key vs non-weighted search on the same strings gave different outputs, which was very counter-intuitive.
@ghost
Copy link

ghost commented Jan 4, 2021

Thanks @luistrivelatto. I'll use your fork and report back.

@comigor
Copy link
Owner

comigor commented Jan 4, 2021

Hello @luistrivelatto, thank you so much for this PR (and sorry for the late reply)!
I'll fix the pipeline and review it soon!

@luistrivelatto luistrivelatto changed the title Fixes #15 Fixes #15, #17, #18, and does other improvements Jan 6, 2021
@luistrivelatto
Copy link
Contributor Author

luistrivelatto commented Jan 6, 2021

Thanks @comigor. Sorry about piling up commits in a single PR, I've changed the title to reflect the changes better.

I'm currently using the lib for tokenized, weighted search, and the results are satisfying. I've gathered 50+ search tests on my data, and only one of them didn't give the expected intuitive results, but I don't consider it a fault of the algorithm because it's due to some use case-specific "gotcha" (a search token ends up being substring of two item tokens, then gives 2 perfect matches when I expected one). I can live with that hehe

Thanks for the package :)

@comigor
Copy link
Owner

comigor commented Jan 6, 2021

@luistrivelatto No problems! Thanks for stepping up and fix those issues!

Before we merge, could you please update the CHANGELOG with all the changes you did?

@luistrivelatto
Copy link
Contributor Author

Sure, done.

@comigor
Copy link
Owner

comigor commented Jan 7, 2021

Thank you very much!

@comigor comigor merged commit df940e3 into comigor:master Jan 7, 2021
@comigor
Copy link
Owner

comigor commented Jan 7, 2021

Published as 0.3.0

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