[MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor by dalmia · Pull Request #7824 · scikit-learn/scikit-learn

dalmia · 2016-11-04T11:03:33Z

Reference Issue

What does this implement/fix? Explain your changes.

Added a separate fit function for DecisionTreeRegressor and DecisionTreeClassifier that calls super to have separate docstrings for both.

maniteja123 · 2016-11-04T11:26:26Z

sklearn/tree/tree.py

+		sample_weight=sample_weight,
+		check_input=check_input,
+		X_idx_sorted=X_idx_sorted)
+


The tests are failing because self is not returned here and also for DecisionTreeRegressor. I suppose you missed to return self in here. Also since the tests are failing, I think it might be good to run flake8 / pep8 checker so that code is pep compliant.

dalmia · 2016-11-04T11:46:56Z

@maniteja123 Thank you for pointing that out. My bad. Also, I ran the pep8 test, but it was showing a few errors mostly on lines that I had not changed, like 'E501 line too long (83 > 79 characters)'. So, for the time being I felt it's better left untouched. However, let me know if you intend to have a discussion on that.

maniteja123 · 2016-11-04T11:50:40Z

Hi @dalmia , you are right that the pep8 errors not related to your changes are better left untouched. But looking at the logs here, I thought it are related to the indentation in your changes.

raghavrv

The point of having a separate fit method is to avoid generic docstrings and have specific ones for regression / classification. Both the docstrings seem identical. Kindly make it more specific...

raghavrv · 2016-11-04T11:58:40Z

sklearn/tree/tree.py


+    def fit(self, X, y, sample_weight=None, check_input=True,
+	    X_idx_sorted=None):
+	"""Build a decision tree classifier from the training set (X, y).


Incorrect indentation?

They should be aligned like

def fit(self, X, y, ... X_idx_sorted): """Build ... Parameters... """

raghavrv · 2016-11-04T11:59:24Z

sklearn/tree/tree.py

+        """
+
+	super(DecisionTreeRegressor,self).fit(
+		X=X,


dalmia · 2016-11-04T13:18:55Z

@raghavrv I modified the generic docstrings for 'y' and 'sample_weight' to a more specific one. Please let me know if I there is any more issue.

dalmia · 2016-11-04T13:20:10Z

@maniteja123 Actually, in my code editor, I used tabs and that messed up everything.

raghavrv · 2016-11-04T13:17:31Z

sklearn/tree/tree.py

+        """
+
+        super(DecisionTreeClassifier,self).fit(
+            X, y, sample_weight, check_input, X_idx_sorted)


Sorry if you misconstrued... I meant X and y alone without the keywords and the rest with keywords

X, y, sampl...=sampl...

dalmia · 2016-11-04T13:29:20Z

@raghavrv No problem!

dalmia · 2016-11-04T15:03:25Z

@raghavrv Please let me know if there is any more issues.

raghavrv · 2016-11-04T15:08:48Z

@amueller 2nd look?

raghavrv · 2016-11-07T19:49:27Z

@jnothman Would you have time for a quick review of this + Merge

dalmia · 2016-11-07T21:03:32Z

Hi @raghavrv, would you also be available for a final review here :
#7818

jnothman

Should we be removing the docstring from BaseDecisionTree.fit? Otherwise LGTM.

jnothman · 2016-11-05T11:14:36Z

sklearn/tree/tree.py

+            to a sparse ``csc_matrix``.
+
+        y : array-like, shape = [n_samples] or [n_samples, n_outputs]
+            The target values (class labels).


might as well specify "as integers or strings"

dalmia · 2016-11-08T09:09:34Z

I feel we shouldn't remove the docstring from BaseDecisionTree.fit. As a user using it for the 1st time, it would be helpful to understand how it generalizes to both regressor and classifier. Please let me know what you think.

nelson-liu · 2016-11-08T09:11:47Z

Indeed, but BaseDecisionTree doesn't show up in the docs, either. I'd personally be in favor of removing it if it is going to be split into these two separate fits.

dalmia · 2016-11-08T09:18:51Z

Yes that's true. I'll make the change then.

jnothman · 2016-11-08T09:48:37Z

I'd like someone else to give their opinion on removing docs from the base class.

amueller · 2016-11-09T18:32:20Z

LGTM

…sionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) * DOC adding separate fit() functions * DOC adding keywords to arguments of super() * DOC removing trailing whitespaces * DOC specifying the type of class labels * DOC removing docstring from BaseDecisionTree.fit

* tag '0.18.1': (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ...

* releases: (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ... Conflicts: removed sklearn/externals/joblib/__init__.py sklearn/externals/joblib/_parallel_backends.py sklearn/externals/joblib/testing.py

* dfsg: (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ...

…sionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) * DOC adding separate fit() functions * DOC adding keywords to arguments of super() * DOC removing trailing whitespaces * DOC specifying the type of class labels * DOC removing docstring from BaseDecisionTree.fit

maniteja123 reviewed Nov 4, 2016

View reviewed changes

raghavrv suggested changes Nov 4, 2016

View reviewed changes

raghavrv changed the title ~~[MRG] DOC adding separate fit() functions for DecisionTreeClassifier and De…~~ [WIP] DOC adding separate fit() methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor Nov 4, 2016

raghavrv added the Documentation label Nov 4, 2016

dalmia closed this Nov 4, 2016

dalmia force-pushed the 7809 branch from d7ebb9a to d7c956a Compare November 4, 2016 13:00

DOC adding separate fit() functions

0010f8a

dalmia reopened this Nov 4, 2016

dalmia changed the title ~~[WIP] DOC adding separate fit() methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor~~ [MRG] DOC adding separate fit() methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor Nov 4, 2016

raghavrv suggested changes Nov 4, 2016

View reviewed changes

DOC adding keywords to arguments of super()

0cba46c

DOC removing trailing whitespaces

d30d38b

raghavrv approved these changes Nov 4, 2016

View reviewed changes

raghavrv changed the title ~~[MRG] DOC adding separate fit() methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor~~ [MRG + 1] DOC adding separate fit() methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor Nov 4, 2016

raghavrv added this to the 0.18.1 milestone Nov 6, 2016

raghavrv added the Waiting for Reviewer label Nov 6, 2016

jnothman requested changes Nov 7, 2016

View reviewed changes

DOC specifying the type of class labels

42c37ea

DOC removing docstring from BaseDecisionTree.fit

3f5fba5

jnothman approved these changes Nov 8, 2016

View reviewed changes

jnothman changed the title ~~[MRG + 1] DOC adding separate fit() methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor~~ [MRG+2] DOC adding separate fit() methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor Nov 8, 2016

amueller merged commit f32d257 into scikit-learn:master Nov 9, 2016

dalmia deleted the 7809 branch November 16, 2016 13:03

Uh oh!

Conversation

dalmia commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

maniteja123 Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia commented Nov 4, 2016 • edited by TomDLT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maniteja123 commented Nov 4, 2016

Uh oh!

raghavrv left a comment

Choose a reason for hiding this comment

Uh oh!

raghavrv Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

raghavrv Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia commented Nov 4, 2016

Uh oh!

dalmia commented Nov 4, 2016

Uh oh!

raghavrv Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia commented Nov 4, 2016

Uh oh!

dalmia commented Nov 4, 2016

Uh oh!

raghavrv commented Nov 4, 2016

Uh oh!

raghavrv commented Nov 7, 2016

Uh oh!

dalmia commented Nov 7, 2016

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman Nov 5, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia commented Nov 8, 2016

Uh oh!

nelson-liu commented Nov 8, 2016

Uh oh!

dalmia commented Nov 8, 2016

Uh oh!

jnothman commented Nov 8, 2016

Uh oh!

amueller commented Nov 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dalmia commented Nov 4, 2016 •

edited

Loading

dalmia commented Nov 4, 2016 •

edited by TomDLT

Loading