glouppe/scikit-learn — pull request #14 by ianozsvald (2013-08-25 14:23:12+00:00)
Title: Various fixes for pull request 2375

Hello. I've spent a few hours reviewing this at the EuroSciPy Sprint, I've not had time to do a full review I'm afraid or check in any changes (I'm not sure how to do that against a PR!). This is against #2375.

With Olivier's help I pulled all the latest code locally and verified that the nosetests test suit fails on this PR. We spent a bit of time on this as TravisCI passes (and the same tests on the 0.15 master branch pass ok). Perhaps there is a misconfiguration with TravisCI?

I've walked through the tests, since I'm short on time I'll just comment here on my fixes:

bagging.py should import "from ..tree import DecisionTreeClassifier, DecisionTreeRegressor" and then use these in BaggingClassifier.__init__ and BaggingRegressor.__init__ to build default base_estimators (exactly as shown in forest.py:ExtraForestClassifier and forest.py:ExtraForestRegressor). The current code does not provide a default base_estimator and so "test_common.py, line 72, in test_all_estimators" fails.

AdaBoostClassifier raises a DataConversionWarning using utils.validation.column_or_1d, this is missing in BaggingClassifier. The result is this test failure:
"test_common.py, line 681, in test_classifiers_input_shapes assert_equal(len(w), 1)"
My quick fix was to add the same call to column_or_1d in BaggingClassifier.fit as it is shown in weight_boosting.py:BaseWeightBoosting.fit (I don't know if this is the *right* thing to do but it does fix the failing test - someone who knows more should verify this is the right choice!).

There are also two pep8 indentation errors.

I have one final test failure which is non-deterministic when I run:
$ nosetests tests/test_common.py
which is presumably due to a random state not being set somewhere.