Skip to content

[WIP] New Feature: Add AverageRegressor #10868

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

Closed
wants to merge 25 commits into from
Closed

[WIP] New Feature: Add AverageRegressor #10868

wants to merge 25 commits into from

Conversation

mohamed-ali
Copy link
Contributor

@mohamed-ali mohamed-ali commented Mar 25, 2018

Reference Issues/PRs

Fixes #10743

What does this implement/fix? Explain your changes.

Following @amueller suggestion about adding sklearn.ensemble.AverageRegressor, this PR implements:

  • sklearn.ensemble.AverageRegressor
  • Associated units tests
  • Docstrings
  • Adds required examples
  • Updates the documentation
  • Updates what's new

Any other comments?

This PR is inspired from work on bagging.py, VotingClassifier and their associated unit-tests.

@mohamed-ali mohamed-ali mentioned this pull request Mar 25, 2018
' tuples')

# validate that at least one estimator is not None
isnone = [clf is not None for _, clf in self.estimators]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we avoid duplication with respect to VotingClassifier?

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also avoid duplicating tests for voting classifier. Can we put them in the same module, e.g. ensemble/voting.py

@mohamed-ali
Copy link
Contributor Author

@jnothman Thanks for reviewing, I refactored the code to voting.py and test_voting.py.

@jnothman
Copy link
Member

jnothman commented Apr 9, 2018 via email

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're still duplicating lots of tests, aren't you? Can you just parameterize some tests with each of VotingClassifier and AverageRegressor?

@mohamed-ali
Copy link
Contributor Author

@jnothman it's still WIP because I haven't finished the examples and the documentation update. Also, I am still not fully happy with the state of the code right now, I need to reduce the amount of duplicated code as much as possible before switching to MRG.

But If you have any other suggestion or comments, that would be helpful. Thanks

@stsouko
Copy link

stsouko commented Sep 30, 2018

Do you plan to complete this PR?
It would be great to see this feature in the next release.

@mohamed-ali
Copy link
Contributor Author

mohamed-ali commented Oct 1, 2018

@stsouko yes, I'll spare few hours this week to finish it.

@stsouko stsouko mentioned this pull request Nov 3, 2018
6 tasks
@mohamed-ali
Copy link
Contributor Author

Closed by #12513

@mohamed-ali mohamed-ali deleted the add-AverageRegressor branch May 9, 2019 13:49
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.

AverageRegressor?
3 participants