Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
@priya-sundaram-dev are the proposed changes useful? |
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
@cclauss I dug into this one. The algorithmic restructuring is genuinely useful — it replaces the old ad-hoc prune() (which counted item tuples and never did true subset pruning) with the textbook Apriori shape: generate_candidates doing the F(k-1) ⋈ F(k-1) join, has_infrequent_subset doing proper (k-1)-subset pruning, and support counted with frozenset.issubset. That's a real correctness/readability win over main.
But it can't merge as-is — two blockers:
-
All doctests were removed.
apriori()and the deletedprune()lost their>>>examples;doctest.testmod()now tests nothing. This repo requires doctests (or a dedicated test) for every function, so we'd be reducing coverage. The refactor needs fresh doctests that pin the new output. -
The return type is inconsistent (and the annotation is wrong). The signature says
-> list[tuple[frozenset, int]], but 1-itemsets are appended as(next(iter(i)), c)→ astrfirst element, while k≥2 itemsets are(sorted(c), count)→ alistfirst element. Running it onload_data()withmin_support=2gives:('milk', 4) # str ('bread', 2) # str (['bread', 'milk'], 2) # listCallers can't consume that uniformly, and it doesn't match the annotation. Also note the output contract changed vs
main(1-itemsets are now included), which should be called out in the PR description.
Suggested path to merge: make the 1-itemset rows (sorted(i), c) so every row is (list[str], int), correct the annotation to list[tuple[list[str], int]], and add doctests for both helpers and apriori(). If the author does that, I'd be happy to re-review — the underlying refactor is worth landing.
Describe your change:
Checklist: