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

[MERGE] fix divide by zero issue in MinHash.contained_by #572

Merged
merged 2 commits into from Dec 5, 2018

Conversation

Projects
None yet
2 participants
@ctb
Member

ctb commented Dec 5, 2018

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?
@codecov

This comment has been minimized.

codecov bot commented Dec 5, 2018

Codecov Report

Merging #572 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #572   +/-   ##
=======================================
  Coverage   88.51%   88.51%           
=======================================
  Files          25       25           
  Lines        3543     3543           
  Branches       37       37           
=======================================
  Hits         3136     3136           
  Misses        407      407

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2e2e3f...93b8343. Read the comment docs.

@@ -364,6 +364,8 @@ cdef class MinHash(object):
"""\
Calculate how much of self is contained by other.
"""
if not self.get_mins():

This comment has been minimized.

@luizirber

luizirber Dec 5, 2018

Member
Suggested change Beta
if not self.get_mins():
if len(self) == 0:

will avoid building mins (len() check the underlying mins in C++ without having to build it)

@ctb

This comment has been minimized.

Member

ctb commented Dec 5, 2018

done!

@luizirber luizirber merged commit 4aab62f into master Dec 5, 2018

3 checks passed

codecov/patch Coverage not affected when comparing d2e2e3f...93b8343
Details
codecov/project 88.51% remains the same compared to d2e2e3f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@luizirber luizirber deleted the fix/divide_by_zero branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment