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

Add Shingle and ShingleSlidingWindow #11

Merged
merged 5 commits into from Nov 21, 2021

Conversation

ShriprajwalK
Copy link
Contributor

Fixes #10
Added two methods, one that slices length n each time and the other uses sliding window. The first one was slightly faster for all values I checked, so I think we should remove the sliding window method.
Sorry for the delay, I wanted to benchmark them both properly but couldn't find the time.

@hbollon
Copy link
Owner

hbollon commented Nov 12, 2021

Hey!
Thanks a lot for this PR, I will try to have a look on it quickly 😄
No problem for the delay mate, it's a pleasure that you contribute!

Shriprajwal K and others added 2 commits November 17, 2021 18:57
test: update unit tests for Cosine/Jaccard with shingle
@hbollon
Copy link
Owner

hbollon commented Nov 19, 2021

Hi @ShriprajwalK !
I updated your PR by adding k-gram shingle support to Cosine / Jaccard similarity functions !
I also add ShingleSlice() func which do the same as Shingle() but return an array instead of a map 😉
Seems good to you? Can I merge?

@ShriprajwalK
Copy link
Contributor Author

Yeah, looks good to me!

@hbollon hbollon merged commit 2ff8354 into hbollon:master Nov 21, 2021
@hbollon
Copy link
Owner

hbollon commented Nov 21, 2021

It's merged 🎉
I will create a new release 1.5.0
Thanks a lot for your contribution mate!

hbollon added a commit that referenced this pull request Nov 21, 2021
* feat: add Shingle function

* test: update unit tests for Cosine/Jaccard with shingle

Co-authored-by: Shriprajwal K <the_daemon_lord@Shriprajwals-MacBook-Air.local>
Co-authored-by: hbollon <hugo.bollon@gmail.com>
@ShriprajwalK ShriprajwalK deleted the spk/shingle branch December 10, 2021 05:14
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.

Missing methods for string shingling
2 participants