[MRG + 1] MLP bug fixes and removing `decision_function` by glennq · Pull Request #5718 · scikit-learn/scikit-learn

glennq · 2015-11-04T20:34:00Z

@amueller
Fix the wrong way of using random_state and mistakenly modifying parameter loss. Some minor fixes and more tests also included.
Remove decision_function to make the design, particularly with _forward_pass cleaner.

BTW, I have already implemented dropout for MLP in my fork. I'm going to make a pull request for that once this one is merged.

amueller · 2015-11-04T21:04:20Z

Thanks. I'm pretty busy with the release but I'll try to review soon.
Travis is failing a doctest because predict_proba changed. It shouldn't have changed for multi-class, only multi-label, right?

btw @kastnerkyle @ogrisel @eickenberg wdyt about removing decision_function ? It seems pretty odd to me.

glennq · 2015-11-04T21:09:16Z

I think doctest has failed because the way we use random_state changed. So coefficient initialization is slightly different.
I don't think it is related to changes in predict_proba. I'll try to rewind the change in predict_proba on my local machine and see if the same failure appears

amueller · 2015-11-04T21:13:46Z

Ah, that's possible. But aren't the weights the first thing that is called with the RNG? Then you need to change the expected output of the doctest (as we just discussed ;)

glennq · 2015-11-04T21:24:31Z

I checkout the commit just after changing use of random_state, and the failure still remains, so I am pretty sure now it's due to change in random state.

I think the problem is that previously check_random_state is called before initializing coefs of every layer...

amueller · 2015-11-04T23:10:09Z

ah, ok. makes sense that it changed.

amueller · 2015-11-20T20:47:17Z

sklearn/neural_network/multilayer_perceptron.py

why is this not needed anymore?

We don't want to change the value of self.loss, and self.loss is actually only used once in _backprop at line 225 in the original code and 223 in the modified version, so I moved the logic of using 'binary_log_loss' instead of 'log_loss' for multilabel case to line 224-225

amueller · 2015-11-20T20:47:35Z

Apart from my comment LGTM. Thanks and sorry for the slow review.

amueller · 2015-11-20T20:47:51Z

Can you please squash your commits? thanks.

amueller · 2015-11-20T20:48:51Z

Could you also add a regression test that checks that the new multi-label behavior is correct that would have failed on the previous version?

glennq · 2015-11-21T01:56:59Z

You mean the previous version that always uses softmax in predict_proba? Sure, I'll add the new test and then squash all commits.

…and added test

amueller · 2016-01-15T23:45:21Z

Sorry for the long silence, I'll try to review this soon.

amueller · 2016-02-09T22:10:41Z

lgtm. @MechCoder wanna review?

glennq · 2016-04-22T21:31:12Z

hi @amueller , just checking if someone's going to review this PR.

raghavrv · 2016-05-15T22:50:51Z

doc/modules/neural_networks_supervised.rst

    >>> clf.predict_proba([[2., 2.], [1., 2.]])  # doctest: +ELLIPSIS
-    array([[ 0.,  1.],
-           [ 0.,  1.]])
+    array([[  1.96718015e-04,   9.99803282e-01],


Could you use ... (ellipsis) instead? Refer this

MechCoder · 2016-09-10T22:16:35Z

Should this be done for the release?

jnothman · 2016-09-11T02:50:55Z

Probably should, since it's changing API of new code, especially given that
it has a +1 already.

On 11 September 2016 at 08:16, Manoj Kumar notifications@github.com wrote:

Should this be done for the release?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#5718 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz6620v5NLWA4OVgkYJwpKDTmUhK6xks5qoyxEgaJpZM4GcEnU
.

ogrisel · 2016-09-12T08:49:35Z

doc/modules/neural_networks_supervised.rst

+in which a sample can belong to more than one class. For each class, the raw
+output passes through the logistic function. Values larger or equal to `0.5`
+are rounded to `1`, otherwise to `0`. For a predicted output of a sample, the
+indices where the value is `1` represents the assigned classes of that sample::


This is not tested in the multilabel test.

ogrisel · 2016-09-12T09:01:16Z

@glennq once my comments are addressed, can you please rebase on top of the current master? As we want to release 0.18rc quickly please let us know if you don't have the time to do that to day and one of us will take care of finishing this PR.

ogrisel · 2016-09-12T09:11:29Z

Actually let me do it.

ogrisel · 2016-09-12T11:09:51Z

Merged by #7393. Thanks @glennq for the fix.

amueller · 2016-09-12T14:28:33Z

@glennq you had a branch for dropout, right?

glennq · 2016-09-12T16:35:41Z

@ogrisel Thank you for merging this fix!
@amueller Yes it's in https://github.com/glennq/scikit-learn/tree/mlp_dropout_new . I can do a rebase and PR this evening if you need that for the release.

amueller · 2016-09-12T16:38:08Z

@glennq we don't need it for the release, but a PR would be nice so we can review. It's a great feature to have.

glennq · 2016-09-12T16:54:13Z

@amueller Sure. Now that this one is merged, I'll make the dropout PR asap.

amueller · 2016-09-12T16:57:45Z

@glennq sweet, thanks :)

glouppe mentioned this pull request Nov 5, 2015

[MRG] MLP: train_test_split is now in model_selection #5712

Closed

glennq changed the title ~~MLP bug fixes and removing decision_function~~ [MRG] MLP bug fixes and removing decision_function Nov 12, 2015

amueller reviewed Nov 20, 2015
View reviewed changes

MLP bug fixes with random state and loss, removed decision_function, …

a3ff02d

…and added test

glennq force-pushed the mlp_fix branch from ef35634 to a3ff02d Compare November 21, 2015 23:44

amueller added the Waiting for Reviewer label Dec 10, 2015

amueller mentioned this pull request Jan 19, 2016

Supporting Dropout for Neural Networks #6175

Closed

amueller changed the title ~~[MRG] MLP bug fixes and removing decision_function~~ [MRG + 1] MLP bug fixes and removing decision_function Feb 9, 2016

raghavrv reviewed May 15, 2016
View reviewed changes

MechCoder added this to the 0.18 milestone Sep 10, 2016

jnothman added the Blocker label Sep 12, 2016

ogrisel reviewed Sep 12, 2016
View reviewed changes

ogrisel mentioned this pull request Sep 12, 2016

[MRG] Fix for MLP multilabel predict_proba and removal of decision_function #7393

Merged

ogrisel closed this Sep 12, 2016

Uh oh!

Conversation

glennq commented Nov 4, 2015

Uh oh!

amueller commented Nov 4, 2015

Uh oh!

glennq commented Nov 4, 2015

Uh oh!

amueller commented Nov 4, 2015

Uh oh!

glennq commented Nov 4, 2015

Uh oh!

amueller commented Nov 4, 2015

Uh oh!

amueller Nov 20, 2015

Choose a reason for hiding this comment

Uh oh!

glennq Nov 21, 2015

Choose a reason for hiding this comment

Uh oh!

amueller commented Nov 20, 2015

Uh oh!

amueller commented Nov 20, 2015

Uh oh!

amueller commented Nov 20, 2015

Uh oh!

glennq commented Nov 21, 2015

Uh oh!

amueller commented Jan 15, 2016

Uh oh!

amueller commented Feb 9, 2016

Uh oh!

glennq commented Apr 22, 2016

Uh oh!

raghavrv May 15, 2016 • 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!

MechCoder commented Sep 10, 2016

Uh oh!

jnothman commented Sep 11, 2016

Uh oh!

ogrisel Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Sep 12, 2016

Uh oh!

ogrisel commented Sep 12, 2016

Uh oh!

amueller commented Sep 12, 2016

Uh oh!

glennq commented Sep 12, 2016

Uh oh!

amueller commented Sep 12, 2016

Uh oh!

glennq commented Sep 12, 2016

Uh oh!

amueller commented Sep 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

raghavrv May 15, 2016 •

edited

Loading

ogrisel commented Sep 12, 2016 •

edited

Loading