-
Notifications
You must be signed in to change notification settings - Fork 875
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
Extend functionality of Sequential Feature Selector to allow repeating cross-validation. #272
base: master
Are you sure you want to change the base?
Conversation
…StratifiedKFold and RepeatedKFold
Hello @nd26! Thanks for updating the PR.
Comment last updated on November 01, 2017 at 15:51 Hours UTC |
Thanks for the PR! Based on the test results, it just seems to be a Tab vs. Whitespace error. I.e., try to replace tabs by 4 whitespaces each. Btw. you can always test your code locally by running |
No problem! Thanks for the reply. Will do tomorrow morning :) |
Sure, no hurry! Btw. added an issue regarding the random number generator you mentioned via email (#274). I am not sure what's more convenient, adding the Repeated CV feature or the random seed first. But I think adding repeated CV (using the current cross_val_score implementation) should work without an extra random seed. After that is implemented, I am happy to rewrite the code to support random seeds. However, I just wanted to mention it in case the repeated CV causes troubles without random seeds (i.e., irreproducible unit tests). In that case, we may want to add random seeds first and then the repeated CV. Anyways, thanks a lot for contributing, both repeated CV and the random seeds are useful enhancements! |
Most likely, if there are unit tests that test the n_cv_repeats parameter, they will fail due to the lack of reproducibility (SFS is highly unstable when used against a large number of features). But, current tests only test cross_val_score without using different values for n_cv_repeats, right? No problem man! |
I just see the reason why one of the unit tests is failing is because one of them is running sklearn 0.18, which doesn't yet have the I think it's fair to switch all the unit tests to sklearn >= 0.19 as it has been out now for a while. The way to fix this would be to change
|
Description
Added ‘n_cv_repeats’ parameter and extended functionality with RepeatedStratifiedKFold and RepeatedKFold to allow repeating cross-validation for both classifiers and regressors respectively.
Related issues or pull requests
Fixes #271
Pull Request requirements
./mlxtend/*/tests
directoriesnosetests ./mlxtend -sv
and make sure that all unit tests passnosetests ./mlxtend --with-coverage
flake8 ./mlxtend
./docs/sources/CHANGELOG.md
filemlxtend/docs/sources/
(optional)