Use native factorization for object string categories - #476
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughObject arrays containing only exact Python strings are sampled and normalized to fixed-width Unicode before categorical factorization. The native factorizer uses the selected array. Mixed, missing, NUL-containing, and near-unique values retain the fallback path. ChangesCategorical factorization
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_custom_ramps_and_palette.py (1)
416-422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert exact fallback labels and codes.
The current assertions only verify structural properties.
len(categories) == len(set(categories))checks a property already enforced by the implementation, andlen(codes) == len(values)does not verify code assignments.Assert the expected
categoriesandcodesfor each parameter. Add astrsubclass or custom string-like case to verify that the exact-type guard preserves custom label semantics.Strengthen the fallback assertions
`@pytest.mark.parametrize`( - "values", + ("values", "expected_categories", "expected_codes"), [ - np.array(["a", None, "a"], dtype=object), - np.array(["a", 1, "a"], dtype=object), - np.array([b"a", b"b", b"a"], dtype=object), + ( + np.array(["a", None, "a"], dtype=object), + ["(missing)", "a"], + [1, 0, 1], + ), + ( + np.array(["a", 1, "a"], dtype=object), + ["1", "a"], + [1, 0, 1], + ), + ( + np.array([b"a", b"b", b"a"], dtype=object), + ["a", "b"], + [0, 1, 0], + ), ], ) -def test_mixed_object_categories_keep_the_fallback(values): +def test_mixed_object_categories_keep_the_fallback( + values, expected_categories, expected_codes +): categories, codes, counts = channels._factorize_categories(values) - assert len(categories) == len(set(categories)) - assert len(codes) == len(values) + assert categories == expected_categories + np.testing.assert_array_equal(codes, expected_codes) assert counts is None🤖 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 `@tests/test_custom_ramps_and_palette.py` around lines 416 - 422, Strengthen test_mixed_object_categories_keep_the_fallback by asserting the exact expected categories and codes instead of only their lengths and uniqueness. Include a custom str subclass or string-like value in the parametrized inputs, and verify that _factorize_categories preserves its custom label semantics through the exact-type guard while counts remains None.
🤖 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.
Inline comments:
In `@python/xy/channels.py`:
- Around line 410-412: Refactor the logic around _normalize_object_strings and
_use_native_fixed_factorizer to probe object-string eligibility and width via a
validation-only _object_string_width helper, without allocating the full Unicode
array. Pass the resulting normalized item size into the probe so its threshold
behavior is preserved, and only perform full normalization after the native path
is selected; keep fallback factorization from retaining an unnecessary Unicode
copy.
- Around line 386-390: Update the string normalization loop around the visible
type check to return None when any string contains a NUL codepoint, allowing the
existing Python fallback to preserve distinct values such as "a" and "a\x00".
Add a regression test covering these two values and verifying they remain
separate categories with correct labels, codes, and counts.
---
Nitpick comments:
In `@tests/test_custom_ramps_and_palette.py`:
- Around line 416-422: Strengthen test_mixed_object_categories_keep_the_fallback
by asserting the exact expected categories and codes instead of only their
lengths and uniqueness. Include a custom str subclass or string-like value in
the parametrized inputs, and verify that _factorize_categories preserves its
custom label semantics through the exact-type guard while counts remains None.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2650696e-d586-43e4-8f59-3c401dda3c84
📒 Files selected for processing (2)
python/xy/channels.pytests/test_custom_ramps_and_palette.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="python/xy/channels.py">
<violation number="1" location="python/xy/channels.py:436">
P3: The two native-factorizer gates on an object column both call `_use_native_fixed_factorizer(arr, ...)` with the same object array, so the bounded probe and its `np.unique` distinct-count are recomputed identically on the same sample for the `sample_width` check and the full `width` check. Since the full-scan width is always >= the sample width, the second gate only re-evaluates the same probe with a different `normalized_itemsize` ratio. Consider computing the probe once (e.g., sampling inside the caller, or precomputing `distinct` and the probe length once) and only re-deriving the `near_unique` ratio for each width, which avoids duplicated work on the hot categorical path this code is explicitly tuned for.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| arr, normalized_itemsize=4 * max(sample_width, 1) | ||
| ): | ||
| width = _object_string_width(arr) | ||
| if width is not None and _use_native_fixed_factorizer( |
There was a problem hiding this comment.
P3: The two native-factorizer gates on an object column both call _use_native_fixed_factorizer(arr, ...) with the same object array, so the bounded probe and its np.unique distinct-count are recomputed identically on the same sample for the sample_width check and the full width check. Since the full-scan width is always >= the sample width, the second gate only re-evaluates the same probe with a different normalized_itemsize ratio. Consider computing the probe once (e.g., sampling inside the caller, or precomputing distinct and the probe length once) and only re-deriving the near_unique ratio for each width, which avoids duplicated work on the hot categorical path this code is explicitly tuned for.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/channels.py, line 436:
<comment>The two native-factorizer gates on an object column both call `_use_native_fixed_factorizer(arr, ...)` with the same object array, so the bounded probe and its `np.unique` distinct-count are recomputed identically on the same sample for the `sample_width` check and the full `width` check. Since the full-scan width is always >= the sample width, the second gate only re-evaluates the same probe with a different `normalized_itemsize` ratio. Consider computing the probe once (e.g., sampling inside the caller, or precomputing `distinct` and the probe length once) and only re-deriving the `near_unique` ratio for each width, which avoids duplicated work on the hot categorical path this code is explicitly tuned for.</comment>
<file context>
@@ -404,11 +418,25 @@ def _factorize_categories(
+ arr, normalized_itemsize=4 * max(sample_width, 1)
+ ):
+ width = _object_string_width(arr)
+ if width is not None and _use_native_fixed_factorizer(
+ arr, normalized_itemsize=4 * max(width, 1)
+ ):
</file context>
1 participant
Summary
Fixes #471
Add a safe native factorization path for homogeneous object-backed
string categories.
Preserve existing fallback behavior for mixed objects, null-like values,
bytes, and custom string semantics.
Keep the existing low-cardinality probe as the fast-path gate.
Preserve category ordering, codes, and count semantics.
Add regression coverage proving only unique labels cross the Python
label path.
Testing
Summary by CodeRabbit
Bug Fixes
Tests