8000 [MRG + 1] Fix MLP batch_size test warning for default value by bhargav · Pull Request #5699 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG + 1] Fix MLP batch_size test warning for default value #5699

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 8, 2016
Merged

[MRG + 1] Fix MLP batch_size test warning for default value #5699

merged 1 commit into from
Mar 8, 2016

Conversation

bhargav
Copy link
Contributor
@bhargav bhargav commented Nov 3, 2015

Initializing batch_size to 'auto' and setting it to n_samples value
for the default case.

@bhargav
Copy link
Contributor Author
bhargav commented Nov 3, 2015

For #5670

@bhargav
Copy link
Contributor Author
bhargav commented Nov 15, 2015

Gentle ping @amueller

@amueller
Copy link
Member

LGTM. Sorry for the delay.

@amueller amueller changed the title Fix MLP batch_size test warning for default value [MRG + 1] Fix MLP batch_size test warning for default value Dec 10, 2015
@mth4saurabh
Copy link
Contributor

@bhargav rebase and push again; appveyor is not failing anymore

@bhargav
Copy link
Contributor Author
bhargav commented Jan 14, 2016

Rebased.

@TomDLT
Copy link
Member
TomDLT commented Jan 14, 2016

So the value 200 is completely gone. Is this ok?
Also, we could add a short description in the docstring explaining the 'auto' behavior.

Otherwise, LGTM

@amueller
Copy link
Member

I think the 200 should be back in. It should have the same behaviour as before.

@bhargav bhargav force-pushed the mlp-test-warning branch 2 times, most recently from 78442c4 to 6731980 Compare February 2, 2016 08:27
@bhargav
Copy link
Contributor Author
bhargav commented Feb 2, 2016

Update PR as per comments. Sorry for the delay.

@amueller
Copy link
Member
amueller commented Mar 7, 2016

Could you please squash the commits? otherwise I think this is good to go. Any other review? @TomDLT ?

@bhargav
Fix MLP batch_size test warning for default value
3d001f3 
Fix default value for batch_size for 'auto' to be min(200, n_samples)
@bhargav bhargav force-pushed the mlp-test-warning branch from 6731980 to 3d001f3 Compare March 7, 2016 23:30
@TomDLT
Copy link
8000
Member
TomDLT commented Mar 8, 2016

LGTM
Travis' failure seems unrelated.

amueller added a commit that referenced this pull request Mar 8, 2016
@amueller
Merge pull request #5699 from bhargav/mlp-test-warning
1d50534 
[MRG + 1] Fix MLP batch_size test warning for default value
@amueller amueller merged commit 1d50534 into scikit-learn:master Mar 8, 2016
@amueller
Copy link
Member
amueller commented Mar 8, 2016

thanks :)

@bhargav bhargav deleted the mlp-test-warning branch March 8, 2016 19:53
@amueller amueller mentioned this pull request Sep 13, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bhargav @amueller @mth4saurabh @TomDLT
0