[MRG] Changed name n_components to n_connected_components in AgglomerativeClustering base class

[scouvreur]

Reference Issues/PRs

This PR is a fix for issue #13357

What does this implement/fix? Explain your changes.

As explained in issue #13357, this fix renames n_components to n_connected_components as n_components refers to something different in the rest of the codebase, but here refers to the attribute of connected components in the graph.

Checked in the following scripts in the codebase for breaking changes:

• sklearn/cluster/tests/test_hierarchical.py
• sklearn/cluster/tests/test_birch.py
• sklearn/cluster/init.py
• sklearn/cluster/birch.py
• examples/cluster/plot_inductive_clustering.py
• examples/cluster/plot_digits_linkage.py
• examples/cluster/plot_linkage_comparison.py
• examples/cluster/plot_cluster_comparison.py
• examples/cluster/plot_agglomerative_clustering_metrics.py
• examples/cluster/plot_ward_structured_vs_unstructured.py
• examples/cluster/plot_agglomerative_clustering.py
• examples/cluster/plot_coin_ward_segmentation.py
• benchmarks/bench_plot_ward.py

and found none.

[jnothman]

To avoid breaking backwards compatibility we need to support users accessing n_components_ but provide a deprecation warning.. The easiest way to do this is to define n_components_ as a property which retrieves n_connected_components_ while raising a warning. I've not yet looked at your test failures.

[scouvreur]

Thanks for your feedback and guidance Joel - I will try and incorporate that idea by adding commits to this PR

[scouvreur]

Hi Joel - I just added the deprecation warning. Let me know if you agree on:

• the scikit-learn versions in the warning (0.20.3 and 0.24 for deprecation)
• the handling of the case where neither n_components nor n_connected_components are passed

[scouvreur]

To avoid breaking backwards compatibility we need to support users accessing n_components_ but provide a deprecation warning.. The easiest way to do this is to define n_components_ as a property which retrieves n_connected_components_ while raising a warning. I've not yet looked at your test failures.

Concerning the test failures, I believe they are not related to this issue however

[jnothman]

If nothing is passed, it should behave the same as version 0.20. we will say deprecated in 0.21 to be removed in 0.23

[jnothman]

The deprecation should be on the attribute retrieval, I.e. in the class:

@property
def n_components_(self):
    Warn...
    return self.n_connected_components_

[scouvreur]

Hi @jnothman - it seems the patch code coverage has decreased, do you know if I should update any of the tests ? Or are the tests not related to this change ?

[jnothman]

You can get code coverage if you add a test that does something like:

with pytest.warns(DeprecationWarning):
    n = my_agglomerative_clusterer.n_components_
assert n == my_agglomerative_clusterer.n_connected_components_

[scouvreur]

Apologies @NicolasHug, I am having some issues with resolving a merge conflict I will look at this tomorrow

[scouvreur]

@NicolasHug just added the whatsnew in commit #31354c3.

Thanks to you @NicolasHug and @jnothman for your feedback and assistance ! I learned like crazy working on this issue.

[scouvreur]

Apologies @NicolasHug @jnothman - after fixing indentation changes in commit #e48f5cc you requested I get a test failure on test_affinity_passed_to_fix_connectivity() in sklearn/cluster/tests/test_hierarchical.py but can't seem to understand why that is the case. Do you think this could be conflicted changes from master I should merge into this branch ?

=================================== FAILURES ============
___________________ test_affinity_passed_to_fix_connectivity ___________________

    def test_affinity_passed_to_fix_connectivity():
        # Test that the affinity parameter is actually passed to the pairwise
        # function
    
        size = 2
        rng = np.random.RandomState(0)
        X = rng.randn(size, size)
        mask = np.array([True, False, False, True])
    
        connectivity = grid_to_graph(n_x=size, n_y=size,
                                     mask=mask, return_as=np.ndarray)
    
        class FakeAffinity:
            def __init__(self):
                self.counter = 0
    
            def increment(self, *args, **kwargs):
                self.counter += 1
                return self.counter
    
        fa = FakeAffinity()
    
        linkage_tree(X, connectivity=connectivity, affinity=fa.increment)
    
>       assert_equal(fa.counter, 3)

FakeAffinity = <class 'sklearn.cluster.tests.test_hierarchical.test_affinity_passed_to_fix_connectivity.<locals>.FakeAffinity'>
X          = array([[ 1.76405235,  0.40015721],
       [ 0.97873798,  2.2408932 ]])
connectivity = array([[1, 0],
       [0, 1]])
fa         = <sklearn.cluster.tests.test_hierarchical.test_affinity_passed_to_fix_connectivity.<locals>.FakeAffinity object at 0x7f5e46075dd8>
mask       = array([ True, False, False,  True], dtype=bool)
rng        = <mtrand.RandomState object at 0x7f5e45f3aee8>
size       = 2

../1/s/sklearn/cluster/tests/test_hierarchical.py:600: 

[NicolasHug]

line 471 in hierarchical should be affinity=affinity

And yes when in doubt, merge master, it never hurts.

[scouvreur]

Thanks alot @NicolasHug, my mistake

[jnothman]

Good work @scouvreur.. Glad you've learnt a lot on the way!

[agramfort]

thx @scouvreur