Improve cpredicates.pyx

[lmores]

Changes:

• Avoid nested for loop in ngrams() function inside cpredicates.pyx.
• Fix docstrings to match actual implementation.
• Add tests.

Not sure how to check runtime improvement, using python benchmarks/benchmarks/canonical.py execution time is 11,xxx seconds both before and after the change.

@fgregg: I am likely to open many more PR like this. Please tell me if you are fine with them. Of course, if I plan to submit bigger changes I will open a thread to discuss them before actually implementing them.

[codecov]

Codecov Report

Base: 73.84% // Head: 73.84% // No change to project coverage 👍

Coverage data is based on head (e88470b) compared to base (baa6071).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1145   +/-   ##
=======================================
  Coverage   73.84%   73.84%           
=======================================
  Files          28       28           
  Lines        2294     2294           
=======================================
  Hits         1694     1694           
  Misses        600      600           

Impacted Files	Coverage Δ
dedupe/predicates.py	83.95% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[fgregg]

the uniqueness guarantee is required

[lmores]

the uniqueness guarantee is required

But was not enforced, right?

[fgregg]

it was enforced by the set object

[fgregg]

hmm that’s true

[lmores]

Sorry, but I don't understand. At the moment I see not set() object inside cpredicates.pyx, in particular ngrams is a list.
I can make it a set if necessary.

[fgregg]

you are right that the current code doesn’t enforce uniqueness, i’ll have to check where that is enforced

[fgregg]

everywhere we call this in predicates.py, we call set on it.

it's a bit silly to do this. let's have cpredicates fill out a set and then not have those set calls in predicates.py.

[lmores]

Actually there is one exception:

class TfidfNGramPredicate(IndexPredicate):
    def preprocess(self, doc: str) -> Sequence[str]:
        return tuple(sorted(ngrams(" ".join(strip_punc(doc).split()), 2)))

But we probably want the ngrams to be unique also here?

[fgregg]

ah.. we actually don't want ngrams to be unique there.

[lmores]

How about the unique_ngrams function I added in the last commit?

[fgregg]

looks good!