[MRG+1] Improves class design for AgglomerativeClustering and FeatureAgglomeration by thechargedneutron · Pull Request #9875 · scikit-learn/scikit-learn

thechargedneutron · 2017-10-05T20:28:47Z

Reference Issue

Fixes #9846

What does this implement/fix? Explain your changes.

Improves the class design of AgglomerativeClustering and FeatureAgglomeration

Any other comments?

Changes will be made if not green.

thechargedneutron · 2017-10-05T21:55:43Z

Need help in what needs to be done to pass the tests in test_non_meta_estimators.

agramfort · 2017-10-06T09:06:58Z

"Improves the class design" is what sense? what is the design issue you identified?

thechargedneutron · 2017-10-13T07:49:20Z

I am not sure if I am on right track but I tried using a _BaseClass, from which the two classes Agglomerative Clustering and Feature Extraction are derived and having pooling_func only in FeatureAgglomeration. I am not sure.

…into aggcluster

thechargedneutron · 2017-10-17T03:54:46Z

@lesteve I guess this is what you were trying to convey. I have made a base class which is used by both AgglomerativeClustering and FeatureAgglomeration. The later one uses pooling_func, not the former.

from sklearn import metrics
from sklearn.datasets.samples_generator import make_blobs
from sklearn.cluster import AgglomerativeClustering


centers = [[1, 1], [-1, -1], [1, -1]]
X, labels_true = make_blobs(n_samples=300, centers=centers, cluster_std=0.5,
                            random_state=0)

model = AgglomerativeClustering(linkage='complete',
                                connectivity=None,
                                affinity = 'cosine',
                                pooling_func = "test_error",
                                n_clusters=3)
model.fit(X)

The above patch now raises an error:

TypeError: __init__() got an unexpected keyword argument 'pooling_func'

jnothman · 2017-10-17T04:17:39Z

I think this is not quite right. You don't need a new base class. FeatureAgglomeration strictly extends from AgglomerativeClustering already, and that is fine. What is a problem is that pooling_func should not be available in AgglomerativeClustering only in descendants of AgglomerationTransform.

We want to remove pooling_func from AgglomerativeClustering and retain it in FeatureAgglomeration. But we cannot merely remove it from AgglomerativeClustering as this may, in an awkward case, break code. So we should deprecate it.

I see the current PR's implementation, however, as more wrong than right. FeatureAgglomeration should define a new init which stores pooling_func and calls AgglomerativeClustering.__init__ with the remaining params. It should also have its own docstring

thechargedneutron · 2017-10-17T04:28:00Z

So, what I can understand from your concern is that, at present, I only need to add a deprecation warning which will raise a warning if a user tries to explicitly call pooling_func. The warning may be raised during fit (or init ??). The rest if fine for atleast two releases. Correct me if I am wrong.

jnothman · 2017-10-17T05:59:52Z

Except that there cannot be a warning in FeatureAgglomeration.__init__, so no, you also need to implement FeatureAgglomeration.__init__ as if pooling_func in AgglomerativeClustering did not exist.

…into aggcluster

jnothman

Please mark the parameter as deprecated in the docstring.

jnothman · 2017-10-18T09:08:21Z

sklearn/cluster/hierarchical.py

        self.linkage = linkage
        self.affinity = affinity
        self.pooling_func = pooling_func
+        if self.pooling_func != np.mean:


This should really be in fit, not in __init__ according to our conventions. And ideally we'd change the default to something like 'deprecated' so that we can warn when the user explicitly passed pooling_func=np.mean

Added the recommended changes, check once

jnothman · 2017-10-18T09:33:31Z

sklearn/cluster/hierarchical.py

          all observations of the two sets.

    pooling_func : callable, default=np.mean
+        deprecated, since AgglomerativeClustering do not use pooling_func


I'd just say "ignored. Deprecate to be removed in ..." And remove the rest as irrelevant

thechargedneutron · 2017-10-19T04:31:39Z

@jnothman Review please.

jnothman

LGTM

jnothman · 2017-10-19T07:40:09Z

sklearn/cluster/hierarchical.py

-        This combines the values of agglomerated features into a single
-        value, and should accept an array of shape [M, N] and the keyword
-        argument ``axis=1``, and reduce it to an array of size [M].
+        ignored, Deprecate to be removed in 0.22.


*deprecated

Look at other places we use the "deprecated" sphinx directive. git grep '\.\. deprecated' will show you plenty of examples.

jnothman · 2017-10-19T07:41:44Z

sklearn/cluster/hierarchical.py

        self
        """
+        if self.pooling_func != 'deprecated':
+            warnings.warn('"pooling_func" as a parameter argument is '


Actually, it surprises me that this has test coverage. Please find out if there is a use of AgglomerativeClustering(pooling_func=...) in the codebase and remove it.

I don't think there's a use of pooling_func. there's no test covering pooling_func.

jnothman · 2017-10-19T07:42:32Z

sklearn/cluster/hierarchical.py

+                 memory=None,
+                 connectivity=None, compute_full_tree='auto',
+                 linkage='ward', pooling_func=np.mean):
+        self.n_clusters = n_clusters


And could you please use super(FeatureAgglomeration, self).__init__(n_clusters=..., ...) instead of setting things individually here?

…into aggcluster

jnothman · 2017-10-19T08:31:18Z

somewhere in tests this warning is raised, says codecov

…

On 19 Oct 2017 7:14 pm, "Kumar Ashutosh" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/cluster/hierarchical.py <#9875 (comment)> : > @@ -694,6 +692,12 @@ def fit(self, X, y=None): ------- self """ + if self.pooling_func != 'deprecated': + warnings.warn('"pooling_func" as a parameter argument is ' I don't think there's a use of pooling_func. there's no test covering pooling_func. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9875 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6wYKs6l95l4sbAzbKMzV09mx31AAks5stwTsgaJpZM4PvpxO> .

lesteve

The main problem is that with your changes as it is FeatureAgglomeration with default parameter has a warning:

from sklearn import metrics
from sklearn.datasets.samples_generator import make_blobs
from sklearn.cluster import AgglomerativeClustering, FeatureAgglomeration


centers = [[1, 1], [-1, -1], [1, -1]]
X, labels_true = make_blobs(n_samples=300, centers=centers, cluster_std=0.5,
                            random_state=0)

model = FeatureAgglomeration()
model.fit(X)

Some other comments too.

lesteve · 2017-10-20T13:27:03Z

sklearn/cluster/hierarchical.py

-        This combines the values of agglomerated features into a single
-        value, and should accept an array of shape [M, N] and the keyword
-        argument ``axis=1``, and reduce it to an array of size [M].
+        ignored, Deprecate to be removed in 0.22.


Look at other places we use the "deprecated" sphinx directive. git grep '\.\. deprecated' will show you plenty of examples.

lesteve · 2017-10-20T13:31:04Z

sklearn/cluster/hierarchical.py

+                                                   connectivity=connectivity,
+                                                   compute_full_tree=compute_full_tree,
+                                                   linkage=linkage, affinity=affinity,
+                                                   pooling_func=pooling_func)


Do not pass pooling_function to the base class otherwise you'll get a warning in AgglomerativeClustering.__init__. This should be fine:

super(FeatureAgglomeration, self).__init__(n_clusters=n_clusters, memory=memory, connectivity=connectivity, compute_full_tree=compute_full_tree, linkage=linkage, affinity=affinity) self.pooling_func=pooling_func

lesteve · 2017-10-20T13:33:50Z

sklearn/cluster/hierarchical.py

+            warnings.warn('"pooling_func" as a parameter argument is '
+                          'deprecated in version 0.20 and will be removed '
+                          'in version 0.22. It is being removed since '
+                          'AgglomerativeClustering do not directly use '


rewording:

Agglomerative 'pooling_func' parameter is not used. It has been deprecated in version 0.20 and will be removed in 0.22.

…into aggcluster

thechargedneutron · 2017-10-21T06:00:59Z

@lesteve Added the changes. I am still not able to figure out the warnings raised by codecov as pointed out by @jnothman . Also, there's a PEP8 failure "line too long". Here:

super(FeatureAgglomeration, self).__init__(n_clusters=n_clusters,
                                                   memory=memory,
                                                   connectivity=connectivity,
                                                   compute_full_tree=compute_full_tree,
                                                   linkage=linkage, affinity=affinity)

Can I modify it in any way?

jnothman · 2017-10-21T11:55:40Z

sklearn/cluster/hierarchical.py

+                 memory=None,
+                 connectivity=None, compute_full_tree='auto',
+                 linkage='ward', pooling_func=np.mean):
+        super(FeatureAgglomeration, self).__init__(n_clusters=n_clusters,


You can just put a newline after ( and indent 4 spaces

jnothman · 2017-10-21T11:58:30Z

sklearn/cluster/hierarchical.py

@@ -644,7 +644,11 @@ class AgglomerativeClustering(BaseEstimator, ClusterMixin):
    pooling_func : callable, default=np.mean
        This combines the values of agglomerated features into a single


No, it does not. This should just say ignored. When @lesteve pointed to other uses of .. deprecated they may be cases where the param is deprecated, but still functional. This one is not.

jnothman · 2017-10-22T03:14:58Z

sklearn/cluster/hierarchical.py

-        This combines the values of agglomerated features into a single
-        value, and should accept an array of shape [M, N] and the keyword
-        argument ``axis=1``, and reduce it to an array of size [M].
+        ignored, Deprecated to be removed in 0.22.


I was not saying that you could or should not use .. deprecated but that leaving in an untrue description of behaviour helped no one.

…into aggcluster

thechargedneutron · 2017-10-23T10:43:37Z

Is this what you were suggesting? @jnothman

jnothman

Yes, that's alright.

lesteve · 2017-10-24T10:12:45Z

I added a whats_new entry. I'll look at the generated doc and then merge if it looks fine.

lesteve · 2017-10-24T14:26:22Z

Merging, thanks a lot!

…cs/donigian-update-contribution-guidelines * 'master' of github.com:scikit-learn/scikit-learn: (23 commits) fixes scikit-learn#10031: fix attribute name and shape in documentation (scikit-learn#10033) [MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633 (scikit-learn#10025) [MRG] Fix LogisticRegression see also should include LogisticRegressionCV(scikit-learn#9995) (scikit-learn#10022) [MRG + 1] Labels of clustering should start at 0 or -1 if noise (scikit-learn#10015) MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573) [MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#10005) [MRG + 1] ENH add check_inverse in FunctionTransformer (scikit-learn#9399) [MRG] FIX bug in nested set_params usage (scikit-learn#9999) [MRG+1] Fix LOF and Isolation benchmarks (scikit-learn#9798) [MRG + 1] Fix negative inputs checking in mean_squared_log_error (scikit-learn#9968) DOC Fix typo (scikit-learn#9996) DOC Fix typo: x axis -> y axis (scikit-learn#9985) improve example plot_forest_iris.py (scikit-learn#9989) [MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClustering (scikit-learn#9875) DOC update news DOC Fix three typos in manifold documentation (scikit-learn#9990) DOC add missing dot in docstring DOC Add what's new for 0.19.1 (scikit-learn#9983) Improve readability of outlier detection example. (scikit-learn#9973) DOC: Fixed typo (scikit-learn#9977) ...

…ering (scikit-learn#9875)

first commit

1ab0c1d

thechargedneutron changed the title ~~[WIP] Improve class design for AgglomerativeClustering and FeatureAgglomeration~~ [WIP] Improves class design for AgglomerativeClustering and FeatureAgglomeration Oct 5, 2017

thechargedneutron added 2 commits October 6, 2017 02:43

super statement corrected

dd5451e

Y changed with y

261f5de

thechargedneutron added 3 commits October 17, 2017 08:27

return statement added to fit function

22bbb9a

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

a6de709

…into aggcluster

init of FeatureAgglomeration corrected

7c682c2

thechargedneutron added 5 commits October 18, 2017 11:49

Changes reverted

fe1d7bc

DeprecationWarning added

705985a

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

260d8cf

…into aggcluster

flake8 failures removed

6aff6a5

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

c5ea897

…into aggcluster

jnothman reviewed Oct 18, 2017

View reviewed changes

thechargedneutron added 2 commits October 18, 2017 14:48

changes added

57a7713

warning shifted to fit

b6d77c4

jnothman approved these changes Oct 18, 2017

View reviewed changes

parameter description changed

9595ec0

thechargedneutron changed the title ~~[WIP] Improves class design for AgglomerativeClustering and FeatureAgglomeration~~ [MRG] Improves class design for AgglomerativeClustering and FeatureAgglomeration Oct 18, 2017

jnothman reviewed Oct 19, 2017

View reviewed changes

jnothman changed the title ~~[MRG] Improves class design for AgglomerativeClustering and FeatureAgglomeration~~ [MRG+1] Improves class design for AgglomerativeClustering and FeatureAgglomeration Oct 19, 2017

jnothman reviewed Oct 19, 2017

View reviewed changes

super statement added

ca9404b

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

f3d87e3

…into aggcluster

lesteve reviewed Oct 20, 2017

View reviewed changes

thechargedneutron added 2 commits October 21, 2017 11:27

changes added

83a397a

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

140e997

…into aggcluster

jnothman reviewed Oct 21, 2017

View reviewed changes

indentation corrected

2eca9ed

jnothman reviewed Oct 22, 2017

View reviewed changes

thechargedneutron added 2 commits October 23, 2017 08:20

parameter doc changed

132f40c

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

9115d39

…into aggcluster

dkmehrmann mentioned this pull request Oct 24, 2017

[MRG] class design for AgglomerativeClustering and FeatureAgglomeration #9846 #9984

Closed

jnothman reviewed Oct 24, 2017

View reviewed changes

lesteve added 2 commits October 24, 2017 12:01

Minor tweak

d53ac99

Add whats_new entry

f135448

lesteve merged commit b4561f0 into scikit-learn:master Oct 24, 2017

thechargedneutron deleted the aggcluster branch October 25, 2017 10:47

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClust…

3349e65

…ering (scikit-learn#9875)

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClust…

4580a77

…ering (scikit-learn#9875)

		@@ -644,7 +644,11 @@ class AgglomerativeClustering(BaseEstimator, ClusterMixin):
		pooling_func : callable, default=np.mean
		This combines the values of agglomerated features into a single

Uh oh!

Conversation

thechargedneutron commented Oct 5, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

thechargedneutron commented Oct 5, 2017

Uh oh!

agramfort commented Oct 6, 2017

Uh oh!

thechargedneutron commented Oct 13, 2017

Uh oh!

thechargedneutron commented Oct 17, 2017

Uh oh!

jnothman commented Oct 17, 2017

Uh oh!

thechargedneutron commented Oct 17, 2017

Uh oh!

jnothman commented Oct 17, 2017 via email

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thechargedneutron commented Oct 19, 2017

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Oct 19, 2017 via email

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thechargedneutron commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thechargedneutron commented Oct 23, 2017

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

lesteve commented Oct 24, 2017

Uh oh!

lesteve commented Oct 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone