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

Implementation of both use_clones and fit_base_estimators #670

Merged
merged 13 commits into from
Apr 4, 2020

Conversation

nilichen
Copy link
Contributor

@nilichen nilichen commented Mar 12, 2020

Description

This PR is to implement both use_clones and fit_base_estimators (previously called refit in EnsembleVoteClassifier) for EnsembleVoteClassifier and StackingClassifier. As well as added corresponding tests for these two parameters.

Will have another PR to address StackingRegressor and StackingCVRegressor.

Related issues or pull requests

Towards #627. See also #383.

Pull Request Checklist

  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file (if applicable)
  • Added appropriate unit test functions in the ./mlxtend/*/tests directories (if applicable)
  • Modify documentation in the corresponding Jupyter Notebook under mlxtend/docs/sources/ (if applicable)
  • Ran PYTHONPATH='.' pytest ./mlxtend -sv and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., PYTHONPATH='.' pytest ./mlxtend/classifier/tests/test_stacking_cv_classifier.py -sv)
  • Checked for style issues by running flake8 ./mlxtend

…eVoteClassifier, StackingClassifier and StackingCVClassifier as well as tests for these to parameters
@pep8speaks
Copy link

pep8speaks commented Mar 12, 2020

Hello @nilichen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 291:80: E501 line too long (80 > 79 characters)

Line 180:80: E501 line too long (82 > 79 characters)

Line 464:80: E501 line too long (89 > 79 characters)

Comment last updated at 2020-04-04 04:33:47 UTC

@nilichen nilichen changed the title [WIP] Implementation of both use_clones and fit_base_estimators Implementation of both use_clones and fit_base_estimators Mar 12, 2020
@rasbt
Copy link
Owner

rasbt commented Mar 13, 2020

Sorry about that. Sure, i can restart it. However, I have seen this a couple of times recently. Let me try a little modification to see if it fixes it.

@nilichen
Copy link
Contributor Author

Thanks! Do you mind taking a look at this PR first? I think it's already a lot of change changes. Gonna make another one for StackingRegressor and StackingCV if this one looks good to you.

@rasbt
Copy link
Owner

rasbt commented Mar 13, 2020

Thanks, and I will look at this soon! Sry, things are a bit crazy here due to the transition to online teaching etc. Will get back to this as soon as the more pressing things got sorted out

@nilichen
Copy link
Contributor Author

nilichen commented Mar 13, 2020 via email

@rasbt
Copy link
Owner

rasbt commented Apr 4, 2020

Thanks again for the PR. I made some minor updates but it looks great overall.

@rasbt rasbt merged commit 17c84d2 into rasbt:master Apr 4, 2020
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

3 participants