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

Adds fit_params support for SequentialFeatureSelector #350

Merged
merged 4 commits into from
Mar 20, 2018

Conversation

zdgriffith
Copy link
Contributor

@zdgriffith zdgriffith commented Mar 19, 2018

Description

Adds support for fit parameters to be passed to the fit method of SequentialFeatureSelector.

Related issues or pull requests

Related to Pull Request #255

Pull Request requirements

  • Added appropriate unit test functions in the ./mlxtend/*/tests directories
  • Ran nosetests ./mlxtend -sv and make sure that all unit tests pass
  • Checked the test coverage by running nosetests ./mlxtend --with-coverage
  • Checked for style issues by running flake8 ./mlxtend
  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file
  • Modify documentation in the appropriate location under mlxtend/docs/sources/ (optional)
  • Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/mlxtend

@pep8speaks
Copy link

pep8speaks commented Mar 19, 2018

Hello @zdgriffith! Thanks for updating the PR.

Line 405:80: E501 line too long (84 > 79 characters)
Line 417:80: E501 line too long (81 > 79 characters)

Comment last updated on March 20, 2018 at 19:39 Hours UTC

@coveralls
Copy link

coveralls commented Mar 19, 2018

Coverage Status

Coverage increased (+0.01%) to 91.745% when pulling 3e7660c on zdgriffith:sfs-fit-params into 4deb65b on rasbt:master.

@rasbt
Copy link
Owner

rasbt commented Mar 20, 2018

This looks good, thanks a lot for the PR! Could you please add an entry to the Changelog and update the API docs?

To update the API docs, just cd into the docs folder and run

python make_api.py

Then, open the Jupyter notebook at mlxtend/docs/sources/user_guide/feature_selection/SequentialFeatureSelector.ipynb, run the last cell, and save it.

For the Changelog, you can add the following to the ## New Features section:

  • The fit method of the SequentialFeatureSelector now optionally accepts **fit_params for the estimator that is used for the feature selection. (#350 by Zach Griffith)

(I am also happy to do that if the "maintainer contrib permissions" are enabled for this PR)

@zdgriffith
Copy link
Contributor Author

Done! Let me know if anything else should be done, otherwise I believe I do have edits from maintainers enabled.

@rasbt
Copy link
Owner

rasbt commented Mar 20, 2018

Looks great, thanks! I don't have any further suggestions, and I'd say it's good to merge :)

@rasbt rasbt merged commit 744012e into rasbt:master Mar 20, 2018
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.

None yet

4 participants