Apply new validation to cuml.random_projection - #8009
Conversation
| @pytest.mark.parametrize("cls", classes) | ||
| @pytest.mark.parametrize("dtype", ["float32", "float64"]) | ||
| def test_components_dtype(cls, dtype): | ||
| def test_components_and_output_dtype(cls, dtype): |
There was a problem hiding this comment.
This test and behavior was incorrect and doesn't match our intended behavior of convert_dtype. I view the change here as a bugfix. Also note that I wrote the code (and test) a few months ago, so I know this wasn't intentional for a specific user need, just a buggy implementation of a standard kwarg.
📝 WalkthroughWalkthroughThis pull request refactors the random projection module to use centralized input validation ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/random_projection/random_projection.py`:
- Around line 89-97: The fit() path currently calls check_inputs(...) with
accept_sparse=True which allows any sparse format but transform() only accepts
CSR/CSC; update the call in random_projection.py (inside the fit method where
check_inputs is invoked) to restrict accept_sparse to the same formats as
transform(), e.g. accept_sparse=("csr", "csc"), so fit(), transform(), and
fit_transform() accept the same sparse matrix types and avoid inconsistency.
In `@python/cuml/tests/test_random_projection.py`:
- Around line 170-177: The test test_components_and_output_dtype only exercises
dense inputs; update it to parametrize over sparse inputs as well (e.g., add a
parameter like "sparse" or "input_format") and when sparse=True convert X to a
scipy.sparse CSR/CSC matrix before calling model = cls(...); ensure you still
call transformed = model.fit_transform(X) and assert both
model.components_.dtype == dtype and transformed.dtype == dtype for the sparse
branch so the sparse conversion/validation path is covered; reference the
existing symbols random_array, classes, test_components_and_output_dtype,
model.fit_transform, model.components_, and transformed when locating where to
add the new param and assertions.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72da559c-8696-4258-921f-557bd8c006fa
📒 Files selected for processing (3)
python/cuml/cuml/random_projection/random_projection.pypython/cuml/tests/test_random_projection.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (1)
- python/cuml/tests/test_sklearn_compatibility.py
viclafargue
left a comment
There was a problem hiding this comment.
Thanks! Just a bunch of questions to draw attention to the input processing in the fit function. Pre-approving should be good to go once solved.
|
/merge |
Fixes #8003.
Note that this fixes a small bug in
fit, bringing the meaning ofconvert_dtypein line with the rest of cuml.