KMeans out of preview namespace #1634
Conversation
md-shafiul-alam
force-pushed
the
msa-kmeans-oop
branch
from
a70f276 to
01183aa
Compare
|
/intelci: run |
| # copied from preview, clusters centers for "full" method are different from "elkan", needs investigation | ||
| - cluster/tests/test_k_means.py::test_kmeans_elkan_results | ||
| - cluster/tests/test_k_means.py::test_unit_weights_vs_no_weights[KMeans-dense] <1.2 | ||
| - cluster/tests/test_k_means.py::test_unit_weights_vs_no_weights[42-KMeans-dense] >=1.2 | ||
| - cluster/tests/test_k_means.py::test_predict_sample_weight_deprecation_warning[KMeans] >=1.3 |
There was a problem hiding this comment.
This should be discussed whether they are a blocker for moving out from preview
There was a problem hiding this comment.
Are we sure they are still failing?
|
Before we merge this, I want to merge master into this branch and rerun CI, not just resolve conflicts. I will review this once it is brought up to date. I've been monkeying around with sklearnex CI a fair bit since the last commit, and want to see how it performs. |
|
/intelci: run |
|
First task will get a green public CI, second step will be to comment out all kmeans-related deselected tests (including GPU) and see what we can reactivate Note, I will delay this because there are some changes which need to be made first, and this will be useless at the moment. |
| # New failing sklearn1.4.1 tests for kmeans associated with incorrect n_iter_ values in daal4py | ||
| - cluster/tests/test_k_means.py::test_relocating_with_duplicates[lloyd-dense] >=1.4 | ||
| - cluster/tests/test_k_means.py::test_relocating_with_duplicates[lloyd-sparse_matrix] >=1.4 | ||
| - cluster/tests/test_k_means.py::test_relocating_with_duplicates[lloyd-sparse_array] >=1.4 | ||
| - cluster/tests/test_k_means.py::test_relocating_with_duplicates[elkan-dense] >=1.4 | ||
| - cluster/tests/test_k_means.py::test_relocating_with_duplicates[elkan-sparse_matrix] >=1.4 | ||
| - cluster/tests/test_k_means.py::test_relocating_with_duplicates[elkan-sparse_array] >=1.4 | ||
|
|
There was a problem hiding this comment.
These are somehow now failing on this PR, but wasn't failing in preview in the main branch. Something unusual is going on, because that shouldn't logically be the case.
| n_iter_, inertia_ = None, None | ||
| labels_, cluster_centers_ = None, None |
There was a problem hiding this comment.
Unless absolutely necessary, these should not be defined as class attributes.
| verbose=0, | ||
| random_state=None, | ||
| copy_x=True, | ||
| algorithm="auto", |
There was a problem hiding this comment.
based off of a PR I just merged, this should fail for sklearn1.1 for init signature matching. Something unusual is going on in testing.
| self._save_attributes() | ||
|
|
||
| @wrap_output_data | ||
| def predict(self, X): |
There was a problem hiding this comment.
This should also fail for a test looking for signature matching of methods. Again, something is not occurring properly in CI.
|
I am glad that the BaseKMeans object has been removed, its really unnecessary. There is a lot of work to be done, it may be good to reference the current version of KMeans in preview to match n_jobs changes etc. There is some work that needs to be done before this is pulled out of preview. @md-shafiul-alam please message me if you have any questions. |
|
The PR is relying on daal4py still, last commit as evidence, likely due to the daal_check_version. This needs to be sorted out first. This can change test coverage and performance, and will likely need be retested/re-benchmarked. |
|
Moved the changes to #1770 |
Moved the changes to #1770