Add normalization to minhash#2247
Conversation
Signed-off-by: Ayush Dattagupta <ayushdg95@gmail.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Greptile SummaryThis PR adds an optional
Confidence Score: 3/5The core normalization logic in
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MinHashStage.process] --> B{normalize_text?}
B -- Yes --> C["_normalize_text(text_series)\nstr.lower() + str.normalize_spaces()"]
C --> D[compute_minhashes]
B -- No --> D
D --> E[write_parquet output]
F[FuzzyDeduplicationWorkflow] --> G["_create_minhash_pipeline()"]
G --> H["MinHashStage(... normalize_text NOT passed ...)"]
H -.->|feature unreachable| I["normalize_text=False (hardcoded default)"]
Reviews (1): Last reviewed commit: "Merge branch 'main' into minhash-normali..." | Re-trigger Greptile |
| normalize_text : bool, default=False | ||
| Whether to normalize text before computing minhashes | ||
| Current normalization is limited to lowercase and trim whitespace |
There was a problem hiding this comment.
The docstring says "trim whitespace" but
str.normalize_spaces() also collapses all interior whitespace sequences (tabs, newlines, multiple spaces) into a single space — not just leading/trailing trimming. The misleading description may cause users to set normalize_text=True expecting only edge-trimming while silently changing their shingled text more aggressively than expected.
| normalize_text : bool, default=False | |
| Whether to normalize text before computing minhashes | |
| Current normalization is limited to lowercase and trim whitespace | |
| normalize_text : bool, default=False | |
| Whether to normalize text before computing minhashes | |
| Current normalization lowercases text and collapses all whitespace | |
| sequences (tabs, newlines, leading/trailing spaces) to a single space |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def _normalize_text(self, text_series: cudf.Series) -> cudf.Series: | ||
| """Normalize text to lowercase and trim whitespace.""" | ||
| return text_series.str.lower().str.normalize_spaces() |
There was a problem hiding this comment.
Same imprecision in the
_normalize_text docstring: "trim whitespace" only describes stripping, while normalize_spaces() also collapses interior whitespace.
| def _normalize_text(self, text_series: cudf.Series) -> cudf.Series: | |
| """Normalize text to lowercase and trim whitespace.""" | |
| return text_series.str.lower().str.normalize_spaces() | |
| def _normalize_text(self, text_series: cudf.Series) -> cudf.Series: | |
| """Normalize text to lowercase and collapse all whitespace sequences to a single space.""" | |
| return text_series.str.lower().str.normalize_spaces() |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Description
Closes #2246.
Adds the option to normalize text (lowercase + whitespace only) to catch more duplicates that differ because of these changes. Will share more stats on dedup rates
Usage
# Add snippet demonstrating usageChecklist