Apply new input validation to metrics.trustworthiness - #8058
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR refactors the trustworthiness() function to replace input_to_cuml_array with check_array/check_consistent_length, extracts device pointers from validated arrays, changes convert_dtype default to True, adjusts neighbor validation to use validated n_samples, updates the backend call, and adds tests covering invalid n_neighbors, mismatched rows, dtype conversion, 1D inputs, and unknown metrics. ChangesTrustworthiness Input Validation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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.
Actionable comments posted: 2
🤖 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/cuml/cuml/metrics/trustworthiness.pyx`:
- Around line 83-104: The code currently calls check_array on X and X_embedded
before validating the requested metric; move or add an early validation of the
metric (e.g., in the trustworthiness function) to raise a clear ValueError for
unsupported metric values before performing any check_array conversions so the
heavy allocations for X_m and X_m2 (and their data.ptr conversions d_X_ptr /
d_X_embedded_ptr) are avoided on invalid input; ensure the validation references
the same metric parameter name used in the function signature and matches the
allowed metric set.
- Around line 105-106: Update the n_neighbors validation in the trustworthiness
implementation so it matches scikit-learn: in the trustworthiness function (in
python/cuml/cuml/metrics/trustworthiness.pyx) replace the current check that
compares n_neighbors to n_samples with a check that raises a ValueError when
n_neighbors >= n_samples / 2 (i.e., require n_neighbors < n_samples / 2); keep
the error message clear and reference n_samples and n_neighbors for context.
🪄 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: Enterprise
Run ID: 3273cf85-51fc-4220-9130-d5693a42f877
📒 Files selected for processing (2)
python/cuml/cuml/metrics/trustworthiness.pyxpython/cuml/tests/test_trustworthiness.py
Drive-by cleanups: * Remove a duplicated 'n_neighbors > X.shape[0]' check. * Validate that X and X_embedded have matching numbers of rows. * Move the n_neighbors check after the input validation so it runs against the validated, coerced array shape.
The docstring stated default=False but the signature defaults to True.
Tighten the existing n_neighbors check to match on the error message, and add tests for: * mismatched row counts between X and X_embedded * convert_dtype=False rejecting non-float32 input * 1D input rejection (ensure_2d via check_array) * unknown metric
1678d35 to
8c1b111
Compare
|
/merge |
Applies new input validation system to
metrics.trustworthiness.Part of #7998