[MRG+1] Fix semi_supervised #9239

musically-ut · 2017-06-28T06:57:49Z

Closes #3550, #5774, #3758, #6727 and deprecates #9192.

Pinging @jnothman, @boechat107, @MechCoder. Am I forgetting someone?

jnothman · 2017-06-29T11:02:21Z

could you please summarise what makes this wip: what is unresolved?

On 28 Jun 2017 4:57 pm, "Utkarsh Upadhyay" ***@***.***> wrote: Closes #3550 <#3550>, #5774 <#5774>, #3758 <#3758>, #6727 <#6727> and deprecates #9192 <#9192>. Pinging @jnothman <https://github.com/jnothman>, @boechat107 <https://github.com/boechat107>, @MechCoder <https://github.com/mechcoder>. Am I forgetting someone? ------------------------------ You can view, comment on, or merge this pull request online at: #9239 Commit Summary - Files for my dev environment with Docker - Fixing label clamping (alpha=0 for hard clamping) - Deprecating alpha, fixing its value to zero - Correct way to deprecate alpha for LabelPropagation - Detailed info for LabelSpreading's alpha parameter - Minor changes in the deprecation message - Improving "deprecated" doc string and raising DeprecationWarning - Using a local "alpha" in "fit" to deprecate LabelPropagation's alpha - Removal of my development files - Using sphinx's "deprecated" tag (jnothman's suggestion) - Deprecation warning: stating that the alpha's value will be ignored - Use __init__ with alpha=None - Merge branch 'master' into lpalpha - Update what's new - Merge pull request #2 from jnothman/lpalpha - Merge branch 'master' into issue-5774 - Try fix RuntimeWarning in test_alpha_deprecation - DOC Indent deprecation details - DOC wording - Update docs - Change to the one true implementation. - Add sanity-checked impl. of Label{Propagation,Spreading} - Raise ValueError if alpha is invalid in LabelSpreading. - Add a normalizing step before clamping to LabelPropagation. File Changes - *M* doc/modules/label_propagation.rst <https://github.com/scikit-learn/scikit-learn/pull/9239/files#diff-0> (4) - *M* doc/whats_new.rst <https://github.com/scikit-learn/scikit-learn/pull/9239/files#diff-1> (5) - *M* examples/semi_supervised/plot_label_propagation_structure.py <https://github.com/scikit-learn/scikit-learn/pull/9239/files#diff-2> (2) - *M* sklearn/semi_supervised/label_propagation.py <https://github.com/scikit-learn/scikit-learn/pull/9239/files#diff-3> (61) - *M* sklearn/semi_supervised/tests/test_label_propagation.py <https://github.com/scikit-learn/scikit-learn/pull/9239/files#diff-4> (66) Patch Links: - https://github.com/scikit-learn/scikit-learn/pull/9239.patch - https://github.com/scikit-learn/scikit-learn/pull/9239.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9239>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz67qW6fQwY3Muzsa3kn9eNbATqrJ8ks5sIflvgaJpZM4OHkEw> .

musically-ut · 2017-06-29T11:08:41Z

Just making Travis happy, I think.

Overall, this is a piecemeal effort to fix semi_supervised module as some other issues will remain.
Nevertheless, I believe it is best to fix these first and then take care of the rest in smaller, more contained, PRs:

Updating the tutorial for semi_supervised learning.
Fixing implementation of kNN kernel [BUG] LabelPropagation should use undirected graphs for knn kernel. #8008.
Adding a few more warnings, re: convergence (e.g. if max_iter is reached) and numerical stability (e.g. if an under/overflow occurs during computations).

I'm fixing the flake8 failures as we speak.

musically-ut · 2017-06-29T13:33:27Z

The failure on appveyor was unrelated to the current changes.

amueller · 2017-06-29T16:09:13Z

can you please merge master? that should get rid of the appveyor error.

The previous way was breaking the test sklearn.tests.test_common.test_all_estimators

Based on the original paper.

This solution isn't great, but it sets the correct value for alpha without violating the restrictions imposed by the tests.

musically-ut · 2017-07-02T09:11:37Z

Updated; awaiting review.

jnothman · 2017-07-02T09:55:25Z

sklearn/semi_supervised/label_propagation.py

@@ -264,13 +280,21 @@ def fit(self, X, y):
            l_previous = self.label_distributions_
            self.label_distributions_ = safe_sparse_dot(
                graph_matrix, self.label_distributions_)
+
+            if alpha == 'propagation':


I think this should be self.variant

Gah!

I wish we had a test to see if normalization was working. :(

jnothman · 2017-07-02T09:55:39Z

sklearn/semi_supervised/label_propagation.py

@@ -264,13 +280,21 @@ def fit(self, X, y):
            l_previous = self.label_distributions_
            self.label_distributions_ = safe_sparse_dot(
                graph_matrix, self.label_distributions_)
+
+            if alpha == 'propagation':
+                # LabelPropagation


and i think this comment is then redundant

jnothman

So is there some way to invent a graph where you get very different normalisation factors for different rows, so that that substantially affects the propagation?

jnothman · 2017-07-02T10:28:41Z

doc/whats_new.rst

+   - Fix :class:`semi_supervised.BaseLabelPropagation` to correctly implement
+     ``LabelPropagation`` and ``LabelSpreading`` as done in the referenced
+     papers. :class:`semi_supervised.LabelPropagation` now always does hard
+     clamping. Its ``alpha`` parameter now defaults to ``None`` and is


I think you should say that "alpha has no effect and is deprecated to be ...".

jnothman · 2017-07-02T10:30:12Z

sklearn/semi_supervised/label_propagation.py

+        if self.variant == 'propagation':
+            # LabelPropagation
+            clamp_weights = np.ones((n_samples, 1))
+            clamp_weights[~unlabeled, 0] = 0.0


This should be the same as clamp_weights = unlabelled.reshape(-1, 1)

jnothman · 2017-07-02T10:30:31Z

sklearn/semi_supervised/label_propagation.py

+            clamp_weights[~unlabeled, 0] = 0.0
+        else:
+            # LabelSpreading
+            clamp_weights = alpha * np.ones((n_samples, 1))


just use clamp_weights = alpha

Have removed clamp_weights completely in favor of keeping only alpha.

jnothman · 2017-07-02T10:33:08Z

sklearn/semi_supervised/label_propagation.py

+                normalizer = np.sum(
+                    self.label_distributions_, axis=1)[:, np.newaxis]
+                self.label_distributions_ /= normalizer
+
            # clamp
            self.label_distributions_ = np.multiply(
                clamp_weights, self.label_distributions_) + y_static


Hmm. In the label prop case, we could just be using np.where(unlabelled, self.label_distributions, y_static) unless I'm mistaken

Yes, np.where(unlabeled[:, np.newaxis], self.label_distributions_, y_static) to be more precise. Have factored out unlabeled[:, np.newaxis] in the actual implementation.

musically-ut · 2017-07-02T17:13:58Z

Err ... the Travis failures, this time, seem to be unrelated to this PR.

ogrisel · 2017-07-03T14:00:34Z

sklearn/semi_supervised/label_propagation.py

@@ -350,6 +373,14 @@ class LabelPropagation(BaseLabelPropagation):
    LabelSpreading : Alternate label propagation strategy more robust to noise
    """

+    variant = 'propagation'


Please make this a private attribute _variant. Otherwise it could be consider public API.

@jnothman I think we can merge once this is made private. Ok on your side?

ogrisel · 2017-07-03T14:02:28Z

You can ignore tests failing with "HTTP Error 500" on mldata.org.

musically-ut · 2017-07-03T14:06:12Z

Updated.

PS: I don't remember whether merely pushing commits sends out an notification e-mail to subscribers by default or not on GitHub (I vaguely recall turning off those notifications for myself somewhere). In case it does, please tell me and I'm sorry about the noise coming from these "Updated" comments.

ogrisel · 2017-07-03T14:08:42Z

It's fine, let's wait for @jnothman's feedback and CI results.

jnothman · 2017-07-03T14:21:53Z

Yes, I'm +1 for merge, because it would be good to finally have this fixed in the release. But it would be nice if we had a non-regression test for that normalisation issue. Want to think about it in the background and try propose something, @musically-ut?

ogrisel · 2017-07-03T14:24:56Z

I will be offline soon. @jnothman I let you handle the end and merging of this PR with @musically-ut :)

musically-ut · 2017-07-03T14:25:26Z

👍

I am going to continue proposing some changes after the underlying algorithm is fixed (e.g. #8008), and will keep trying to come up with better tests in the background.

@ogrisel Thanks for the review!

jnothman · 2017-07-03T22:20:35Z

Thanks and well done, @musically-ut, @boechat107, @MechCoder and all.

amueller · 2017-07-05T15:24:04Z

AWESOME! thank you so much to everybody working on this!

* Files for my dev environment with Docker * Fixing label clamping (alpha=0 for hard clamping) * Deprecating alpha, fixing its value to zero * Correct way to deprecate alpha for LabelPropagation The previous way was breaking the test sklearn.tests.test_common.test_all_estimators * Detailed info for LabelSpreading's alpha parameter Based on the original paper. * Minor changes in the deprecation message * Improving "deprecated" doc string and raising DeprecationWarning * Using a local "alpha" in "fit" to deprecate LabelPropagation's alpha This solution isn't great, but it sets the correct value for alpha without violating the restrictions imposed by the tests. * Removal of my development files * Using sphinx's "deprecated" tag (jnothman's suggestion) * Deprecation warning: stating that the alpha's value will be ignored * Use __init__ with alpha=None * Update what's new * Try fix RuntimeWarning in test_alpha_deprecation * DOC Indent deprecation details * DOC wording * Update docs * Change to the one true implementation. * Add sanity-checked impl. of Label{Propagation,Spreading} * Raise ValueError if alpha is invalid in LabelSpreading. * Add a normalizing step before clamping to LabelPropagation. * Fix flake8 errors. * Remove duplicate imports. * DOC Update What's New. * Specify alpha's value in the error. * Tidy up tests. Add a test and add references, where needed. * Add comment to non-regression test. * Fix documentation. * Move check for alpha into fit from __init__. * Fix corner case of LabelSpreading with alpha=None. * alpha -> self.variant * Make Whats_new more explicit. * Simplify impl. of Label{Propagation,Spreading}. * variant -> _variant.

* Files for my dev environment with Docker * Fixing label clamping (alpha=0 for hard clamping) * Deprecating alpha, fixing its value to zero * Correct way to deprecate alpha for LabelPropagation The previous way was breaking the test sklearn.tests.test_common.test_all_estimators * Detailed info for LabelSpreading's alpha parameter Based on the original paper. * Minor changes in the deprecation message * Improving "deprecated" doc string and raising DeprecationWarning * Using a local "alpha" in "fit" to deprecate LabelPropagation's alpha This solution isn't great, but it sets the correct value for alpha without violating the restrictions imposed by the tests. * Removal of my development files * Using sphinx's "deprecated" tag (jnothman's suggestion) * Deprecation warning: stating that the alpha's value will be ignored * Use __init__ with alpha=None * Update what's new * Try fix RuntimeWarning in test_alpha_deprecation * DOC Indent deprecation details * DOC wording * Update docs * Change to the one true implementation. * Add sanity-checked impl. of Label{Propagation,Spreading} * Raise ValueError if alpha is inv 179B alid in LabelSpreading. * Add a normalizing step before clamping to LabelPropagation. * Fix flake8 errors. * Remove duplicate imports. * DOC Update What's New. * Specify alpha's value in the error. * Tidy up tests. Add a test and add references, where needed. * Add comment to non-regression test. * Fix documentation. * Move check for alpha into fit from __init__. * Fix corner case of LabelSpreading with alpha=None. * alpha -> self.variant * Make Whats_new more explicit. * Simplify impl. of Label{Propagation,Spreading}. * variant -> _variant.

musically-ut changed the title ~~[WIP] Fix semi supervised~~ Fix semi supervised Jun 29, 2017

ogrisel mentioned this pull request Jun 29, 2017

[WIP] Try finish #6727: alpha deprecation in LabelPropagation #9192

Closed

ogrisel added this to the 0.20 milestone Jun 29, 2017

ogrisel modified the milestones: 0.19, 0.20 Jun 29, 2017

musically-ut changed the title ~~Fix semi supervised~~ [MRG] Fix semi_supervised Jun 29, 2017

boechat107 and others added 18 commits June 29, 2017 18:15

Files for my dev environment with Docker

cb2bbed

Fixing label clamping (alpha=0 for hard clamping)

cbde082

Deprecating alpha, fixing its value to zero

979d872

Correct way to deprecate alpha for LabelPropagation

262f758

The previous way was breaking the test sklearn.tests.test_common.test_all_estimators

Detailed info for LabelSpreading's alpha parameter

7e3a2ed

Based on the original paper.

Minor changes in the deprecation message

03311bf

Improving "deprecated" doc string and raising DeprecationWarning

9a68a41

Using a local "alpha" in "fit" to deprecate LabelPropagation's alpha

c356a82

This solution isn't great, but it sets the correct value for alpha without violating the restrictions imposed by the tests.

Removal of my development files

76310e5

Using sphinx's "deprecated" tag (jnothman's suggestion)

ce15e24

Deprecation warning: stating that the alpha's value will be ignored

e863810

Use __init__ with alpha=None

356ccc2

Update what's new

1506b1b

Try fix RuntimeWarning in test_alpha_deprecation

5508a24

DOC Indent deprecation details

31b1be1

DOC wording

6d8cefc

Update docs

6a8ba66

Change to the one true implementation.

6ee6dd1

Fix corner case of LabelSpreading with alpha=None.

77174e8

jnothman reviewed Jul 2, 2017

View reviewed changes

alpha -> self.variant

81c0964

jnothman reviewed Jul 2, 2017

View reviewed changes

musically-ut added 2 commits July 2, 2017 17:43

Make Whats_new more explicit.

8f21418

Simplify impl. of Label{Propagation,Spreading}.

105bcf7

ogrisel reviewed Jul 3, 2017

View reviewed changes

variant -> _variant.

f8fa824

jnothman merged commit 0dc2279 into scikit-learn:master Jul 3, 2017

This was referenced Jul 5, 2017

BUG: clamping is implemented incorrectly in sklearn.semi_supervised.LabelSpreading #5774

Closed

[FIX] Fixing Issue #3550 - hard clamping. #3758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Fix semi_supervised #9239

[MRG+1] Fix semi_supervised #9239

[MRG+1] Fix semi_supervised #9239

[MRG+1] Fix semi_supervised #9239

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment