Fix index misalignment in char_wb ngram padding - #8429
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe vectorizer now preserves document alignment for character n-grams, including documents with zero tokens. Related test annotations now reference the NVIDIA/cuml repository. ChangesCharacter n-gram alignment fix
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change corrects char_wb n-gram document alignment and adds parity coverage for the reported multi-character range. No unresolved merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/cuml/feature_extraction/_vectorizers.py (1)
216-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an enabled
char_wbTfidf regression test.In
python/cuml/tests/test_text_feature_extraction.py, testTfidfVectorizer.fit_transform(analyzer="char_wb", ngram_range=(2, 6)). Compare the sparse output and feature names with scikit-learn usingget_feature_names_out().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuml/cuml/feature_extraction/_vectorizers.py` around lines 216 - 224, Add an enabled regression test in test_text_feature_extraction.py covering TfidfVectorizer with analyzer="char_wb" and ngram_range=(2, 6). Fit and transform the same input with cuML and scikit-learn, then compare sparse outputs and feature names using get_feature_names_out().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cuml/cuml/feature_extraction/_vectorizers.py`:
- Around line 216-224: Add an enabled regression test in
test_text_feature_extraction.py covering TfidfVectorizer with analyzer="char_wb"
and ngram_range=(2, 6). Fit and transform the same input with cuML and
scikit-learn, then compare sparse outputs and feature names using
get_feature_names_out().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80ca255e-7d3d-41f0-ab51-594df4f04c33
📒 Files selected for processing (1)
python/cuml/cuml/feature_extraction/_vectorizers.py
|
@divyegala Can you review this, please? |
divyegala
left a comment
divyegala
left a comment
There was a problem hiding this comment.
Thank you for the contribution! Could you please add a test?
|
Added |
|
/ok to test 36f9500 |
|
/merge |
|
Hi @adityaanikam , could you please run pre-commit and push up a new commit? |
done |
|
/ok to test 41a6044 |
CI on this PR failed test_tfidf_vectorizer_char_wb_ngrams with KeyError: boolean label can not be used without a boolean index, in get_ngrams' ngram_count[not_empty_docs] filter. That test's DOCS fixture includes two empty-string documents, and this is the first char_wb test to include one.
get_char_ngrams' char_wb branch computes ngram_count via doc_id_df.groupby("doc_id", sort=True).sum(). A document that tokenizes to zero tokens never contributes a row to doc_id_df, so its doc_id is entirely absent from the groupby result's index, rather than present with a count of 0. token_count, computed separately via str_series.str.token_count(), still has an entry for every document. The two indices then disagree, and get_ngrams' later not_empty_docs boolean mask, built from token_count's full index, no longer aligns with ngram_count's shorter one.
Reindexed ngram_count onto token_count's index with fill_value=0 right after the groupby, restoring the same index for both that the other two branches (char, ngram_size == 1) already get for free since their ngram_count is arithmetic directly on token_count.
This bug predates this PR and is unrelated to the padding order change in get_char_ngrams; it was only ever latent because no earlier char_wb test included a document with zero tokens. The regression test already added in this PR (test_tfidf_vectorizer_char_wb_ngrams) exercises it directly, so no additional test is needed. Could not run this locally, cuml requires an NVIDIA GPU and CUDA runtime; verified the fix by tracing the exact index mismatch against the failing job's log and reasoning through cudf.Series.reindex semantics, and am relying on CI to confirm.
|
/merge |
|
/ok to test dd84c4f |
6 participants
Fixes #8416
Root cause
get_char_ngrams()pads each token withself.delimiteron both sides before generating character ngrams: