Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed layer number and learning rate init to make execution of plot_mnist_filters.py quicker #21647

Merged
merged 5 commits into from Nov 23, 2021

Conversation

ghost
Copy link

@ghost ghost commented Nov 12, 2021

#21598 @adrinjalali

Tuned down the number of layers and adapted the learning_rate_init and max_iter params to make it between 15 and 20 sec quicker with almost no quality loss and without changing the learning effect or character of the example.
mnist_before
mnist_after

@adrinjalali adrinjalali changed the title Changed layer number and learning rate init to make execution of example quicker Changed layer number and learning rate init to make execution of plot_mnist_filters.py quicker Nov 13, 2021
@adrinjalali adrinjalali mentioned this pull request Nov 13, 2021
41 tasks
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the improvement.

@ogrisel
Copy link
Member

ogrisel commented Nov 15, 2021

While you are at it, could you please edit the comments to expand the CI acronym from "because of CI’s time constraints" to "because of resource usage constraints on our Continuous Integration infrastructure that is used to build this documentation on a regular basis."

@adrinjalali
Copy link
Member

The comment in the code says that this doesn't converge, does it still not converge?

There's also a mistake in the comment, there's an extra "are" there, you can fix that too.

Also, please mention the absolute times before and after.

@ghost
Copy link
Author

ghost commented Nov 15, 2021

@ogrisel Edited your improved explanation into the docstring

@ghost
Copy link
Author

ghost commented Nov 15, 2021

@adrinjalali Yes, rest should be good, will delete the additional "are" and also paste in some new run times

@ghost
Copy link
Author

ghost commented Nov 15, 2021

@adrinjalali @ogrisel I deleted the additional "are" and added @ogrisel`s comment also in that line since it was the same as in the docstring, so I figured to put the new explanation in both lines (docstring and single line comment)

@adrinjalali
Copy link
Member

I think we can get a better speedup here if we use a much smaller part of the data as training set. Could you please use train_test_split, and use a small part of the data as training, while somewhat keeping the output the same?

@ghost
Copy link
Author

ghost commented Nov 16, 2021

@adrinjalali Okay, I'll get on it, soon :)

@ghost
Copy link
Author

ghost commented Nov 16, 2021

@adrinjalali We get the following results:

With this code

X_train, X_test = X[:60000], X[60000:]
y_train, y_test = y[:60000], y[60000:]

we get this result:
bef

and with this code

X_train, X_test, y_train, y_test = train_test_split(X, y, random_state=0, test_size=0.7)

we get this result:

aft

So, you were right. train_test_split is great. I tried going with less than 30 % train data but it doesn't get much faster, only the validation accuracy gets slightly worse (at least different from the original behavior)...

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, you okay with the changes @ogrisel ?

@adrinjalali
Copy link
Member

Looking at the changes since @ogrisel 's approval, I think we can merge, thanks @sveneschlbeck

@adrinjalali adrinjalali merged commit 11222e0 into scikit-learn:main Nov 23, 2021
@ghost ghost deleted the speed_increased_example_mnist branch November 23, 2021 17:34
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021
…plot_mnist_filters.py quicker (scikit-learn#21647)

* Changed layer number and learning rate init to make execution of example faster

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
…plot_mnist_filters.py quicker (scikit-learn#21647)

* Changed layer number and learning rate init to make execution of example faster

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
…plot_mnist_filters.py quicker (scikit-learn#21647)

* Changed layer number and learning rate init to make execution of example faster

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py
glemaitre pushed a commit that referenced this pull request Dec 25, 2021
…plot_mnist_filters.py quicker (#21647)

* Changed layer number and learning rate init to make execution of example faster

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py

* Update plot_mnist_filters.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants