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

Fix MinHash + Add Scalable BF #28 + Optimize Partitionned BF #53

Merged
merged 11 commits into from
Mar 26, 2022
Merged

Conversation

folkvir
Copy link
Collaborator

@folkvir folkvir commented Mar 23, 2022

  • Fix MinHash, comparison was wrong due to a unique lodash filter.
  • Optimize the Partitionned Bloom Filter with a list of BitSet, import is still doable with the old structure to the new one.
    • Remove the length function
  • Add Scalable Bloom Filter Implements Scalable Bloom Filter #28

A major bump is required.

@folkvir folkvir requested a review from Callidon March 23, 2022 12:48
@folkvir
Copy link
Collaborator Author

folkvir commented Mar 23, 2022

@Callidon Any idea to recover the length function from Partitionned Bloom Filter?
The PR is in draft until everything is ok, changelog, documentation, minor changes, etc...

Copy link
Owner

@Callidon Callidon left a comment

Choose a reason for hiding this comment

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

I have some comments and suggested changes, but otherwise it looks great. It will be a nice additions to the library, and fixes some impactful bugs. Good job 👏 👍

PS: I need to think a bit more about the length function, but IMO, it's not an issue it disappears right now. We always were borderline with this kind of helper functions, as they are not from the original algorithms. I'm not against removing them, unless there is a real use case from user feedback. However, since we are removing a feature, we will need to release a minor or a major version after the merge.

README.md Outdated Show resolved Hide resolved
src/bloom/partitioned-bloom-filter.ts Outdated Show resolved Hide resolved
src/bloom/scalable-bloom-filter.ts Outdated Show resolved Hide resolved
src/bloom/scalable-bloom-filter.ts Show resolved Hide resolved
src/bloom/scalable-bloom-filter.ts Show resolved Hide resolved
src/bloom/scalable-bloom-filter.ts Show resolved Hide resolved
src/bloom/scalable-bloom-filter.ts Outdated Show resolved Hide resolved
src/bloom/scalable-bloom-filter.ts Outdated Show resolved Hide resolved
src/sketch/min-hash.ts Outdated Show resolved Hide resolved
@Callidon Callidon linked an issue Mar 25, 2022 that may be closed by this pull request
Copy link
Owner

@Callidon Callidon left a comment

Choose a reason for hiding this comment

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

Thank for the changes, all is good for me, we can merge it alongside #54 for the next release

@folkvir folkvir marked this pull request as ready for review March 26, 2022 13:21
@folkvir folkvir merged commit 1d37e81 into master Mar 26, 2022
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.

Implements Scalable Bloom Filter
2 participants