But most specialised CV implementations (LogisticRegressionCV, ElasticNetCV, ...) don't have estimator..?

At this point, I don't see us gaining much from such a refactoring.

No, you can't share ducktyping delegation so easily, nor docstrings.

But most specialised CV implementations (LogisticRegressionCV, ElasticNetCV, ...) don't have estimator..?

I was thinking we could add one to unify the interface of EstimatorCV... (As we also would like to have cv_results_ in them)?

@raghavrv can you maybe focus on things that are release relevant for this week and the next? That would be very helpful. Thanks :)

Yes ofcourse! This PR was started just as I mentioned here and as I noted at the PR description... For discussion after release of 0.18 not for now... ;)

BTW @amueller just to clarify everything currently tagged with 0.18 is release critical correct? Or are there false positives in tagging?

there might be false positives ;)

and false negatives

@jnothman @amueller Could we revisit this now?

@amueller And responding to your earlier question in the other PR

would the new base-class also be used in other CV objects?

Currently only BaseSearchCV and in the near future GradientBoostingCV can use this... If this proposal goes well, BaseForestCV would also use it...

How much negative lines does that yield?

Right now since this just refactors the Mixin class out of GridSearchCV into base.py, this adds 30 lines, but with GradientBoostingCV reusing it, it could save about 100 lines of redundant code...

Additionally we can use this to design our validation API?

I'm not too familiar with this part of the code. What is the explicit benefit of this change?

@raghavrv I want to get out 0.18.1 first ;)
Also: why wouldn't the other existing CV estimators use it? What's different about them compared to the Random Forest and Gradient Boosting?

@raghavrv I want to get out 0.18.1 first ;)

Deja vu ;) But yes I'm working on it. :)

Please, @raghavrv, can you add to the description precisely what you propose to be shared/provided by this mixin?

Also: why wouldn't the other existing CV estimators use it? What's different about them compared to the Random Forest and Gradient Boosting?

For instance LogisticRegressionCV does not use best_estimator_ nor does it need predict as it inherits from LogisticRegression. (Maybe we should do the same for GradientBoostingCV too instead of having this mixin?)

can you add to the description precisely what you propose to be shared/provided by this mixin?

When I opened this I thought it would be nice to refactor the predict.* functions (with if_delegate_has_method decorators) from GridSearchCV and BaseGradientBoostingCV into a separate mixin (EstimatorCVMixin).

Now after Andy brought up the question of a separate validation set instead of cross-validation.

I thought maybe we could use this mixin to specify a new method for passing the validation set explicitly...

def validated_fit(X_train, y_train, X_validate, y_validate):
    X_all = np.vstack(X_train, X_validate)
    y_all = np.vstack(y_train, y_validate)
    test_folds = np.hstack((np.zeros(X_train.shape[0]), np.ones(X_test.shape[0]))))
    self.cv = PredefinedSplit(test_folds)
    return self.fit(X_all, y_all)

(Not sure how sensible that train of thought was...)

I am -1 for adding new methods. And I don't think there's enough shared
between *CV to benefit from sharing the delegated methods.

On 26 October 2016 at 00:01, Raghav RV notifications@github.com wrote:

Also: why wouldn't the other existing CV estimators use it? What's
different about them compared to the Random Forest and Gradient Boosting?

For instance LogisticRegressionCV does not use best_estimator_ nor does
it need predict as it inherits from LogisticRegression. (Maybe we should
do the same for GradientBoostingCV too instead of having this mixin?)

can you add to the description precisely what you propose to be
shared/provided by this mixin?

When I opened this I thought it would be nice to refactor the predict.*
functions (with if_delegate_has_method decorators) from GridSearchCV and
BaseGradientBoostingCV
#7071 into a separate
mixin (EstimatorCVMixin).

Now after Andy brought up the question of a separate validation set
instead of cross-validation.

I thought maybe we could use this mixin to specify an abstract method

def validated_fit(X_train, y_train, X_validate, y_validate):
X_all = np.vstack(X_train, X_validate)
y_all = np.vstack(y_train, y_validate)
test_folds = np.hstack((np.zeros(X_train.shape[0]), np.ones(X_test.shape[0]))))
self.cv = PredefinedSplit(test_folds)
return self.fit(X_all, y_all)

(Not sure how sensible that train of thought was...)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7305 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz68byP2WgQHKMjaGBxhA-qwdFgfLUks5q3f2QgaJpZM4Jxq9Z
.

Thanks for the comment :) I'll leave this closed and try to subclass from GradientBoosting for that PR...