-
Notifications
You must be signed in to change notification settings - Fork 1
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 BM25 and BagOfWords transformers, update tests, update readme, re… #12
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #12 +/- ##
==========================================
+ Coverage 81.05% 88.33% +7.28%
==========================================
Files 3 7 +4
Lines 95 180 +85
==========================================
+ Hits 77 159 +82
- Misses 18 21 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pazzo83 for this very substantial contribution 🎉 .
I have reviewed the PR with mainly the MLJ model API in mind.
@storopoli Are you able to look over some of the core algorithm implentation details? @pazzo83 Be great if you can flag the names of those core functions.
Yes I can, but only beginning next week (Final academic semester week is hell now). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is great.
Only the unusual double spacing after a period at the .md
files and docstrings.
Was it intentional?
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Co-authored-by: Jose Storopoli <43353831+storopoli@users.noreply.github.com>
Adding two new transformers with updated tests and readme,
Also some refactoring since there is some shared code now.