[MRG+1] - DeprecationWarning for n_components parameter for linkage_tree by sharanry · Pull Request #9309 · scikit-learn/scikit-learn

sharanry · 2017-07-10T02:05:56Z

Reference Issue

Fixes #9307

What does this implement/fix? Explain your changes.

Adds DeprecationWarning to n_components parameter which is no longer being used in linkage_tree function.

Any other comments?

No.

Update from base Jul-04-2017

Change 10JUL2017

jnothman · 2017-07-10T04:18:14Z

Needs a test, thanks!

…into issue-9307

sharanry · 2017-07-10T05:38:35Z

@jnothman
children, n_nodes, n_leaves, parent = assert_warns(UserWarning, linkage_tree, n_components=10)

Why can't i use this as a test?

jnothman · 2017-07-10T06:48:08Z

sklearn/cluster/tests/test_hierarchical.py

    assert_raises(ValueError, linkage_tree, X, connectivity=np.ones((4, 4)))

+    # Test for warning of deprecation of n_components in linkage_tree
+    children, n_nodes, n_leaves, parent = assert_warns(UserWarning, linkage_tree, n_components=10)


Put it in a separate test function, please. You should probably assert that output is identical to without the n_components.

jnothman · 2017-07-10T08:33:25Z

sklearn/cluster/tests/test_hierarchical.py

+            with ignore_warnings():
+                    # Test for warning of deprecation of n_components in linkage_tree
+                    children, n_nodes, n_leaves, parent = assert_warns(DeprecationWarning, linkage_tree,this_X.T, n_components=10)
+                    children1, n_nodes1, n_leaves1, parent1 = linkage_tree(this_X.T, n_components=10)


What's the point in this line?

Without it, you shouldn't need the ignore_warnings

To compare the output with and without n_components. To make sure it's same.. is it not required ?

The previous line will calculate the same. As long as we believe assert_warns works, and we do

i have removed the lines and i have made it check only for the warning.

jnothman

Almost there

jnothman · 2017-07-10T11:45:11Z

sklearn/cluster/tests/test_hierarchical.py

+def test_deprecation_of_n_components_in_linkage_tree():
+    rng = np.random.RandomState(0)
+    X = rng.randn(50, 100)
+    for tree_builder in _TREE_BUILDERS.values():


This variable is unused in the loop

jnothman · 2017-07-10T11:45:28Z

sklearn/cluster/tests/test_hierarchical.py

+    rng = np.random.RandomState(0)
+    X = rng.randn(50, 100)
+    for tree_builder in _TREE_BUILDERS.values():
+        for this_X in (X, X[0]):


Testing with X alone is fine

jnothman · 2017-07-10T11:45:51Z

sklearn/cluster/tests/test_hierarchical.py

    assert_raises(ValueError, linkage_tree, X, linkage='foo')
    assert_raises(ValueError, linkage_tree, X, connectivity=np.ones((4, 4)))

+


Please remove this blank line

sharanry · 2017-07-10T14:09:00Z

@jnothman All checks have passed. Please take a look. TIA!

jnothman · 2017-07-10T21:25:51Z

sklearn/cluster/tests/test_hierarchical.py

+    X = rng.randn(50, 100)
+    # Test for warning of deprecation of n_components in linkage_tree
+    children, n_nodes, n_leaves, parent = assert_warns(
+                                        DeprecationWarning,


This indentation is a bit unconventional

jnothman · 2017-07-10T21:26:31Z

sklearn/cluster/tests/test_hierarchical.py

+                                        linkage_tree,
+                                        X.T,
+                                        n_components=10)
+


I had suggested comparing children, n_nodes, n_leaves, parent to linkage_tree(X.T)

jnothman · 2017-07-10T21:27:15Z

sklearn/cluster/hierarchical.py

    --------
    ward_tree : hierarchical clustering with ward linkage
    """
+    if n_components is not None:


To be sure, we should really be testing against a different value, like if n_components != 'deprecated' and setting the default to 'deprecated' so that a user explicitly passing None will receive the warning.

jnothman

LGTM

TomDLT · 2017-07-18T10:18:48Z

Thanks @sharanry

…ree (scikit-learn#9309) * Depreciation warning for n_components in sklearn/cluster/hierarchical.py * typo fix * Whitespace fix * Update hierarchical.py * Added test * Added test-v2 * Test for deprecation * Test function name change * Updated test * Update test_hierarchical.py * fixing flake8 errors * removed blank line * modifying test * Change condition for deprecation warning * Change indentation and compare function output * fix flake8 errors * Update test_hierarchical.py

sharanry added 6 commits July 4, 2017 16:16

Merge pull request #1 from scikit-learn/master

d97cff3

Update from base Jul-04-2017

Merge pull request #2 from scikit-learn/master

d5a1e40

Change 10JUL2017

Depreciation warning for n_components in sklearn/cluster/hierarchical.py

05887e4

typo fix

6ba0553

Whitespace fix

ecbe994

Update hierarchical.py

3a0af31

sharanry added 3 commits July 10, 2017 10:21

Added test

9351173

Merge branch 'issue-9307' of https://github.com/sharanry/scikit-learn …

07644fc

…into issue-9307

Added test-v2

602fcaa

jnothman reviewed Jul 10, 2017

View reviewed changes

sharanry added 2 commits July 10, 2017 12:54

Test for deprecation

69ad939

Test function name change

ab549b8

jnothman reviewed Jul 10, 2017

View reviewed changes

sharanry added 3 commits July 10, 2017 15:34

Updated test

24f466e

Update test_hierarchical.py

0dd8c8f

fixing flake8 errors

07c4c83

jnothman reviewed Jul 10, 2017

View reviewed changes

sharanry added 2 commits July 10, 2017 17:25

removed blank line

bc9f031

modifying test

c751c63

jnothman reviewed Jul 10, 2017

View reviewed changes

sharanry changed the title ~~Issue 9307: DeprecationWarning for n_components parameter for linkage_tree~~ [MRG] - DeprecationWarning for n_components parameter for linkage_tree Jul 10, 2017

sharanry added 4 commits July 11, 2017 05:35

Change condition for deprecation warning

dd2e116

Change indentation and compare function output

e0293c3

fix flake8 errors

59dd8fe

Update test_hierarchical.py

0b38229

jnothman reviewed Jul 11, 2017

View reviewed changes

jnothman changed the title ~~[MRG] - DeprecationWarning for n_components parameter for linkage_tree~~ [MRG+1] - DeprecationWarning for n_components parameter for linkage_tree Jul 11, 2017

TomDLT merged commit bd4953d into scikit-learn:master Jul 18, 2017

amueller mentioned this pull request Aug 11, 2017

deprecation of n_components happened in 0.19 not 0.18 #9527

Merged

		assert_raises(ValueError, linkage_tree, X, linkage='foo')
		assert_raises(ValueError, linkage_tree, X, connectivity=np.ones((4, 4)))

Uh oh!

Conversation

sharanry commented Jul 10, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

jnothman commented Jul 10, 2017

Uh oh!

sharanry commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sharanry Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 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!

sharanry commented Jul 10, 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!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

TomDLT commented Jul 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sharanry commented Jul 10, 2017 •

edited

Loading

sharanry Jul 10, 2017 •

edited

Loading