Apply new input validation to cuml.multiclass - #8017
Conversation
|
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)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces legacy input conversion helpers with centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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.
🧹 Nitpick comments (1)
python/cuml/cuml/multiclass/multiclass.py (1)
50-58: Regarding your question on dtype enforcement:The current approach of enforcing
dtype=("float32", "float64")at the wrapper level is reasonable—it provides early validation with clear error messages before data reaches the underlying estimator. While this may result in some validation overlap with the estimator's owncheck_inputs, the overhead is minimal and the explicit contract makes the wrapper's requirements clear to users.If you wanted to relax this, using
dtype=Nonewould defer entirely to the estimator, but you'd lose the early error messaging at the wrapper boundary. The current implementation is a sound default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/multiclass/multiclass.py` around lines 50 - 58, The wrapper currently enforces dtype=("float32", "float64") via check_inputs in multiclass.py (call to check_inputs(self, X, y, dtype=(...))), which is acceptable for early validation; leave it as-is for explicit input contracts, or if you prefer to defer to the underlying estimator remove the dtype restriction by changing dtype=("float32", "float64") to dtype=None in the check_inputs call so the estimator's own validation handles dtype instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuml/cuml/multiclass/multiclass.py`:
- Around line 50-58: The wrapper currently enforces dtype=("float32", "float64")
via check_inputs in multiclass.py (call to check_inputs(self, X, y,
dtype=(...))), which is acceptable for early validation; leave it as-is for
explicit input contracts, or if you prefer to defer to the underlying estimator
remove the dtype restriction by changing dtype=("float32", "float64") to
dtype=None in the check_inputs call so the estimator's own validation handles
dtype instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0bbb1663-9314-4e87-9930-c2e565590654
📒 Files selected for processing (1)
python/cuml/cuml/multiclass/multiclass.py
|
/merge |
Part of #7428, fixes #7999.
Question:
check_inputsinfit/predict/decision_functioncurrently enforcesdtype=("float32", "float64"). Since the underlying estimator also callscheck_inputswith its own dtype constraints, should the wrapper enforce dtype at all, or passdtype=Noneand let the estimator handle it?