Add EmpiricalCovariance estimator - #8074
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
de74267 to
24dc0c8
Compare
|
/ok to test d8a9b38 |
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces ChangesEmpiricalCovariance GPU-Accelerated Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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/__init__.py`:
- Line 25: The top-level import of EmpiricalCovariance was added but not
exported in the module's public API; update the module's __all__ list to include
"EmpiricalCovariance" so it is part of the top-level exports. Locate the __all__
variable in cuml.__init__.py (where other estimators like "LedoitWolf" are
listed) and add "EmpiricalCovariance" to that sequence, ensuring string spelling
matches the imported symbol.
In `@python/cuml/tests/test_empirical_covariance.py`:
- Around line 291-321: Add an explicit empty-input test covering n_samples=0 to
match sklearn behavior: create X = _make_random_data(n_samples=0, n_features=3)
and call EmpiricalCovariance().fit(X) and SklearnEmpiricalCovariance().fit(X);
then assert they exhibit the same outcome (either both raise the same exception
via pytest.raises(...) or both return compatible covariance_ arrays via
np.testing.assert_allclose). Reference the existing tests
(test_single_feature_matches_sklearn, test_single_sample_warns,
test_more_features_than_samples_matches_sklearn) and use the same patterns
(store_precision flag if needed) to place the new test near those edge-case
tests.
🪄 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: 744d464a-95e4-4c02-92a6-dabc8cd8230b
📒 Files selected for processing (11)
docs/source/api/cuml.covariance.rstdocs/source/api/index.rstdocs/source/cuml-accel/faq.rstpython/cuml/cuml/__init__.pypython/cuml/cuml/accel/_overrides/sklearn/covariance.pypython/cuml/cuml/covariance/__init__.pypython/cuml/cuml/covariance/empirical_covariance.pypython/cuml/cuml_accel_tests/integration/test_empirical_covariance.pypython/cuml/tests/test_base.pypython/cuml/tests/test_empirical_covariance.pypython/cuml/tests/test_sklearn_compatibility.py
|
/merge |
Adds
cuml.covariance.EmpiricalCovariancewith sklearn-compatiblefit,score,get_precision,error_norm, andmahalanobisbehavior, and wires it intocuml.accelforsklearn.covariance.EmpiricalCovariance.Includes direct estimator tests, sklearn compatibility coverage, cuml.accel integration coverage, API docs, and FAQ coverage.
Closes #7525