Skip to content

[MRG + 1] Fix reference in fetch_kddcup99 based on #7861 #8071

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

Merged
merged 4 commits into from
Dec 19, 2016

Conversation

b-carter
Copy link
Contributor

Reference Issue

Changes reference for fetch_kddcup99 based on #7861.

What does this implement/fix? Explain your changes.

Updates docstring reference.

@maniteja123
Copy link
Contributor

Hi, looking at the issue, there was also first point regarding the default value of percent10. Could you also change the documentation and perhaps also the default value in _fetch_brute_kddcup99 helper function ? Thanks !

@b-carter
Copy link
Contributor Author

Sure, I'll update the pull request to reflect those changes as well.

@b-carter
Copy link
Contributor Author

I think it makes sense to leave the default value of percent10 as False (so calling the function without specifying the parameter downloads the complete data).

Let me know if you think this should be the other way and I can update the PR. Otherwise, I think it is good to go.

@maniteja123
Copy link
Contributor

Hi, thanks for updating this. I think there was a discussion at #5946 here for changing it to True. But I suppose one of the core devs can give a better idea.

@b-carter
Copy link
Contributor Author

Good point, if one of the core devs can chime in about which default value it should have, I can update it.

@lesteve
Copy link
Member

lesteve commented Dec 19, 2016

LGTM

@lesteve lesteve changed the title Fix reference in fetch_kddcup99 based on #7861 [MRG + 1] Fix reference in fetch_kddcup99 based on #7861 Dec 19, 2016
@lesteve
Copy link
Member

lesteve commented Dec 19, 2016

Actually reading #5946 (comment) it seems like the idea is to have percent10=True by default. I tried and the data is indeed quite big. On my machine it took all the RAM I had left (~14GB) and my system started swapping.

I reckon the documentation should be changed rather than the default value. As @maniteja123 said if you could change _fetch_brute_kddcup99 to have a consistent default and documentation for percent10, that would be great.

@b-carter
Copy link
Contributor Author

Updated!

@lesteve
Copy link
Member

lesteve commented Dec 19, 2016

Thanks a lot, merging!

@lesteve lesteve merged commit f61d96c into scikit-learn:master Dec 19, 2016
sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
* fix 'percent10' parameter default in fetch_kddcup99 docstring
* Consistent default 'percent10' value in _fetch_brute_kddcup99 to be consistent
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
* fix 'percent10' parameter default in fetch_kddcup99 docstring
* Consistent default 'percent10' value in _fetch_brute_kddcup99 to be consistent
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
* fix 'percent10' parameter default in fetch_kddcup99 docstring
* Consistent default 'percent10' value in _fetch_brute_kddcup99 to be consistent
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
* fix 'percent10' parameter default in fetch_kddcup99 docstring
* Consistent default 'percent10' value in _fetch_brute_kddcup99 to be consistent
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
* fix 'percent10' parameter default in fetch_kddcup99 docstring
* Consistent default 'percent10' value in _fetch_brute_kddcup99 to be consistent
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.

3 participants