[MRG+1] Throw an error with explicit message if n_estimators is not an integer. by fabianegli · Pull Request #7457 · scikit-learn/scikit-learn

fabianegli · 2016-09-19T12:26:32Z

Reference Issue

Example: Fixes #7146

What does this implement/fix? Explain your changes.

Added type checking of n_estimators and made the corresponding error message explicit about the type issue.

I used the Type checking currently proposed in issue #7394

Any other comments?

I got the intended behavior when testing with rf.fit(n_estimators=100), which works as expeced, and rf.fit(n_estimators='100') which throws the type error as expected.

Example error message with traceback:

Traceback (most recent call last):
File "", line 1, in
File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/sklearn/ensemble/forest.py", line 247, in fit
self._validate_estimator()
File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/sklearn/ensemble/base.py", line 61, in _validate_estimator
"got {0}.".format(type(self.n_estimators)))
ValueError: n_estimators must be an integer, got <class 'str'>.

raghavrv · 2016-09-19T12:50:49Z

Could you close this one and merge this commit onto your other PR (#7454). Both can be reviewed in one go...

fabianegli · 2016-09-19T13:00:09Z

Couldn't figure out how to merge pull requests. I closed the old pull request. Since only whitespace characters were changed/deleted to comply with pep8 there is nothing lost by closing the first pull request, which does not have to be reviewed at all any more.

raghavrv · 2016-09-19T13:07:03Z

While you are in this branch (issue_7146_pep8), do git cherry-pick abd95d3. That will bring the changes brought about by your recently closed PR (#7454) into this active PR.

fabianegli · 2016-09-19T13:17:36Z

Ah, cool, thank you for teaching me how to 'cherry-pick'! There is nothing in the closed pull request that I want to migrate though. Everything that should be here from the other branch is already in this pull request.
This might be bad practice, but I just checked out the master branch again, made a new branch, added the improved pep8-complying changes again. All because of my lack of 'cherry-pick'ing skills... I am sorry for the mess/confusion.
I wonder: Is it a big nogo to close a pull request and completely forget about it?

raghavrv · 2016-09-19T13:40:11Z

No problem at all. Thanks for the work! I didn't know you did that already...

Now you will have to add the tests and ping us again.

fabianegli · 2016-09-19T14:41:57Z

Now I understand how this works with the pull requests and latter pushing to the same branch which contains the pull request. It works even smoother than I imagined!

TomDLT · 2016-09-19T14:53:46Z

Thanks for adding a test, but it fails on your branch because of a typo.

fabianegli · 2016-09-19T14:59:43Z

Will it work if I amend the commit in the branch locally and push it 'again'?

TomDLT · 2016-09-19T15:12:39Z

If you add a new commit, you can push normally.
If you amend an commit that has already been pushed, you will need to "force push", with the option git push -f, but you should use it wisely as it is a strong source of problems.

fabianegli · 2016-09-19T15:13:34Z

Just made a new commit. No forcing done.

raghavrv · 2016-09-19T15:35:41Z

sklearn/ensemble/tests/test_base.py

+def test_base_not_int_n_estimators():
+    # Check that instantiating a BaseEnsemble with a string as n_estimators raises
+    # a ValueError requesting n_estimators to be supplied as an integer.
+    ensemble_string = BaggingClassifier(base_estimator=Perceptron(), n_estimators='3')


Can you

check if this works for np.int32(3)

check if this raises an error for 3.0

fabianegli · 2016-09-20T09:57:35Z

I wonder, is there an easy way to build scikit-learn on Mac OS X?
python setup.py install --prefix /path/to/prefix did run, but testing with nosetests failed utterly on me with a message that scikit learn seems to have been built incorrectly and the following error message: ImportError: No module named 'sklearn.__check_build._check_build'

TomDLT · 2016-09-20T13:35:29Z

Be sure to remove all scikit-learn installation in your environment (check with pip list and/or conda list)
Install with python setup.py develop

More info in the contributing guide.

amueller · 2016-10-07T17:16:44Z

needs a rebase.

fabianegli · 2016-10-14T14:58:51Z

Is there anything else that needs to be done before this PR can be merged?

TomDLT · 2016-10-14T15:04:57Z

LGTM

TomDLT · 2016-10-14T15:08:04Z

I wonder how the squash_and_merge GitHub function will deal with the rebase commit (d0666e4).
Yet the diff looks good, so I guess it might works.

amueller · 2016-10-14T19:55:35Z

sklearn/ensemble/tests/test_base.py

    # Check that instantiating a BaseEnsemble with n_estimators<=0 raises
    # a ValueError.
-    ensemble = BaggingClassifier(base_estimator=Perceptron(), n_estimators=0)
+    ensemble = BaggingClassifier(base_estimator=Perceptron(),


why the newline?

amueller · 2016-10-14T19:56:57Z

@TomDLT rebase commit? I don't think there's such a thing...

amueller · 2016-10-14T19:58:47Z

thanks!

…n integer. (scikit-learn#7457) * Throw an error with explicit message if n_estimators is not an integer. * Testing for explicit message if n_estimators is not an integer. * Fixed typo in test for explicit message if n_estimators is an integer. * Added tests for np.int32 and float input. * pep8 compliance * fix function name * Import numpy to test n_estimators suplied as numpy int32.

Throw an error with explicit message if n_estimators is not an integer.

f3533a0

Testing for explicit message if n_estimators is not an integer.

0a4bf16

Fixed typo in test for explicit message if n_estimators is an integer.

24a0379

raghavrv reviewed Sep 19, 2016

View reviewed changes

Added tests for np.int32 and float input.

652c161

raghavrv approved these changes Sep 19, 2016

View reviewed changes

fabianegli added 3 commits September 20, 2016 00:30

pep8 compliance

c73f7a6

fix function name

05c1dcd

Import numpy to test n_estimators suplied as numpy int32.

1f7c367

rebased to current master

d0666e4

TomDLT changed the title ~~Throw an error with explicit message if n_estimators is not an integer.~~ [MRG+1] Throw an error with explicit message if n_estimators is not an integer. Oct 14, 2016

amueller reviewed Oct 14, 2016

View reviewed changes

amueller merged commit 53e6381 into scikit-learn:master Oct 14, 2016

fabianegli deleted the issue_7146_pep8 branch October 14, 2016 20:13

naoise-h mentioned this pull request Aug 22, 2022

FIX remove n_estimators checks from validate_estimator #24224

Merged

Uh oh!

Conversation

fabianegli commented Sep 19, 2016

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

raghavrv commented Sep 19, 2016

Uh oh!

fabianegli commented Sep 19, 2016

Uh oh!

raghavrv commented Sep 19, 2016

Uh oh!

fabianegli commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raghavrv commented Sep 19, 2016

Uh oh!

fabianegli commented Sep 19, 2016

Uh oh!

TomDLT commented Sep 19, 2016

Uh oh!

fabianegli commented Sep 19, 2016

Uh oh!

TomDLT commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabianegli commented Sep 19, 2016

Uh oh!

raghavrv Sep 19, 2016

Choose a reason for hiding this comment

Uh oh!

fabianegli commented Sep 20, 2016

Uh oh!

TomDLT commented Sep 20, 2016

Uh oh!

amueller commented Oct 7, 2016

Uh oh!

fabianegli commented Oct 14, 2016

Uh oh!

TomDLT commented Oct 14, 2016

Uh oh!

TomDLT commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amueller Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

amueller commented Oct 14, 2016

Uh oh!

amueller commented Oct 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fabianegli commented Sep 19, 2016 •

edited

Loading

TomDLT commented Sep 19, 2016 •

edited

Loading

TomDLT commented Oct 14, 2016 •

edited

Loading