DOC Fixed Plot Mnist Example by marimeireles · Pull Request #16200 · scikit-learn/scikit-learn

marimeireles · 2020-01-25T10:42:49Z

Fixes #14117
Fixes the warning about the MLPClassifier not being able to converge, now it does.

@adrinjalali, @noatamir

adrinjalali · 2020-01-25T13:57:22Z

Could you also say how long it takes not to run the example, and what the output is compared to what's on master?

marimeireles · 2020-01-25T14:58:40Z

Hi @adrinjalali

So this is my results after I made the model converge:

And it took 70.39416408538818 seconds.

Without any changes:

And it takes 28.29195475578308 seconds to run.

Which is pretty much expected, IMO. Since I had to increase the number of steps so it could converge.
Also, comparing the before and after they're somewhat alike. What do you think?
Is is good enough or should I try to make it more alike?

Thanks for the feedback :)

glemaitre

On the CI, it takes already 1 minute. I don't think that we should increase this time. I think that we state in the example the following:

To make the example run faster, we use very few hidden units, and train only for a very short time. Training longer would result in weights with a much smoother spatial appearance.

Thus, I would improve the discussion stating that we don't let the neural-network converge (which will issue a warning).

glemaitre · 2020-01-26T10:27:24Z

examples/neural_networks/plot_mnist_filters.py

 y_train, y_test = y[:60000], y[60000:]

-mlp = MLPClassifier(hidden_layer_sizes=(50,), max_iter=10, alpha=1e-4,
+mlp = MLPClassifier(hidden_layer_sizes=(100,), max_iter=40, alpha=1e-4,


Why increasing the number of hidden layers?

I asked around the sprint and nobody could explain me why there was a 50 in the example so I decided it was better to just use the default in the docs.
No other reason, I can change it =)

This is to limit the complexity of the model to have a faster training from what I can see in the narration.

marimeireles · 2020-01-26T20:49:00Z

If @adrinjalali doesn't see a problem with it than I'll do it.

adrinjalali · 2020-01-28T11:30:40Z

Yeah we need to have it much faster than what it is now. It'll make the CI time out if we have a few of these slow examples. @glemaitre is also another core developer like me, you can trust him ;)

I'd say we can say in the example that for performance reasons we have it like this, and that it will raise a warning, and catch the warning in the code so that running the example doesn't show the warning.

Let us know if you need any help with any of that :)

marimeireles · 2020-01-28T11:59:19Z

@adrinjalali haha it's not that I don't trust him, it's because I pinged and talked to you first.
Okay! I'll revert it and add a message.
Thanks for your input!

glemaitre · 2020-01-28T13:23:04Z

Catching the warning could be a solution with a nice comment explaining why we do so. Beter to repeat ourself :)

adrinjalali · 2020-02-11T09:55:21Z

You could also catch the warning the way we do it in plot_mlp_training_curves.py, with a comment:

        # some parameter combinations will not converge as can be seen on the
        # plots so they are ignored here
        with warnings.catch_warnings():
            warnings.filterwarnings("ignore", category=ConvergenceWarning,
                                    module="sklearn")
            mlp.fit(X, y)

cmarmo · 2020-03-02T15:54:18Z

@marimeireles the failing check could probably be solved by a synchronisation with upstream. Could you please do this? Thanks a lot!

cmarmo · 2020-04-15T13:36:16Z

Hi @marimeireles, hope you are doing well in those difficult times... are you still interested in finishing this PR? Thanks for the work you already did!

marimeireles · 2020-04-16T13:10:34Z

Hey @cmarmo, thanks for reaching out again, I lost this in my backlog.
I'm fine! Hope you're okay too.
Lemme give a try on this now =)

thomasjpfan

Thank you for the PR @marimeireles !

thomasjpfan · 2020-04-16T19:36:58Z

examples/neural_networks/plot_mnist_filters.py

-smoother spatial appearance.
+smoother spatial appearance. The example will throw a warning because it
+doesn't converge, in this case this is what we want because of CI's time
+constringements.


Did you mean the following?

Suggested change

constringements.

constraints.

thomasjpfan · 2020-04-16T19:37:16Z

examples/neural_networks/plot_mnist_filters.py

                    solver='sgd', verbose=10, random_state=1,
                    learning_rate_init=.1)

+# this example won't converge because of CI's constrigements, so we catch the


Did you mean the following?

Suggested change

# this example won't converge because of CI's constrigements, so we catch the

# this example won't converge because of CI's time constraints, so we catch the

thomasjpfan · 2020-04-16T19:37:38Z

examples/neural_networks/plot_mnist_filters.py

+with warnings.catch_warnings():
+    warnings.filterwarnings("ignore", category=ConvergenceWarning,
+                            module="sklearn")
+
 mlp.fit(X_train, y_train)


The fit needs to be indented to catch

Suggested change

with warnings.catch_warnings():

warnings.filterwarnings("ignore", category=ConvergenceWarning,

module="sklearn")

mlp.fit(X_train, y_train)

with warnings.catch_warnings():

warnings.filterwarnings("ignore", category=ConvergenceWarning,

module="sklearn")

mlp.fit(X_train, y_train)

marimeireles · 2020-04-17T06:21:49Z

Would you like me to rebase these commits?

cmarmo · 2020-04-17T09:58:08Z

Would you like me to rebase these commits?

Thanks @marimeireles! No need to rebase: core-devs will stash the commits when merging.
@scikit-learn/core-devs, this is the last example to solve #14117: someone available for reviewing? Thanks!

adrinjalali

Nice, thanks @marimeireles

thomasjpfan

Thank you @marimeireles !

marimeireles changed the title ~~Fixes #14117~~ The Plot Misnt Example now converges Jan 25, 2020

marimeireles changed the title ~~The Plot Misnt Example now converges~~ The Plot Mnist Example now converges Jan 25, 2020

marimeireles changed the title ~~The Plot Mnist Example now converges~~ Fixed Plot Mnist Example Jan 25, 2020

glemaitre requested changes Jan 26, 2020

View reviewed changes

cmarmo mentioned this pull request Mar 2, 2020

CI: scikit-image 0.12.3 requires dask[array]>=0.5.0, which is not installed. #16545

Closed

cmarmo added the Needs work label Apr 15, 2020

marimeireles added 3 commits April 16, 2020 15:17

Explain why the plot_mnist_filters isn't expected to converge

0c81ae9

Catch the non convergence

971cbf5

Add missing headers

a374880

marimeireles requested a review from glemaitre April 16, 2020 17:37

thomasjpfan reviewed Apr 16, 2020

View reviewed changes

Remove words that I made up and fix indentation

f25163c

cmarmo added Waiting for Reviewer and removed Needs work labels Apr 17, 2020

adrinjalali approved these changes Apr 17, 2020

View reviewed changes

thomasjpfan approved these changes Apr 17, 2020

View reviewed changes

thomasjpfan changed the title ~~Fixed Plot Mnist Example~~ DOC Fixed Plot Mnist Example Apr 17, 2020

thomasjpfan merged commit 269afa3 into scikit-learn:master Apr 17, 2020

cmarmo removed the Waiting for Reviewer label Apr 29, 2020

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

DOC Fixed Plot Mnist Example (scikit-learn#16200)

c3bb966

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020

DOC Fixed Plot Mnist Example (scikit-learn#16200)

1957e63

	# this example won't converge because of CI's constrigements, so we catch the
	# this example won't converge because of CI's time constraints, so we catch the

Uh oh!

Conversation

marimeireles commented Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali commented Jan 25, 2020

Uh oh!

marimeireles commented Jan 25, 2020

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

marimeireles Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre Jan 28, 2020 • edited by ogrisel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marimeireles commented Jan 26, 2020

Uh oh!

adrinjalali commented Jan 28, 2020

Uh oh!

marimeireles commented Jan 28, 2020

Uh oh!

glemaitre commented Jan 28, 2020

Uh oh!

adrinjalali commented Feb 11, 2020

Uh oh!

cmarmo commented Mar 2, 2020

Uh oh!

cmarmo commented Apr 15, 2020

Uh oh!

marimeireles commented Apr 16, 2020

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

marimeireles commented Apr 17, 2020

Uh oh!

cmarmo commented Apr 17, 2020

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marimeireles commented Jan 25, 2020 •

edited

Loading

glemaitre Jan 28, 2020 •

edited by ogrisel

Loading