Skip to content

results_ attribute for all EstimatorCV classes? #7206

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.

Sign up for GitHub

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

Jump to bottom
Closed
13 tasks
raghavrv opened this issue Aug 18, 2016 · 14 comments · Fixed by #28915
Closed
13 tasks

results_ attribute for all EstimatorCV classes? #7206

raghavrv opened this issue Aug 18, 2016 · 14 comments · Fixed by #28915

Comments

@raghavrv
Copy link
Member

raghavrv commented Aug 18, 2016 •
edited
Loading

In light of #6697

  1. Should we have a BaseEstimatorCV class defining the required parameters (like results_, best_estimators_) as an interface attribute that can be overwritten?
  2. Should the parameter still be called results_ and not search_results_ - Let's call it cv_results_ would be more representative of what it holds.

TODO

  • calibration.py:class CalibratedClassifierCV(BaseEstimator, ClassifierMixin)
  • covariance/graph_lasso_.py:class GraphLassoCV(GraphLasso):
  • feature_selection/rfe.py:class RFECV(RFE, MetaEstimatorMixin):
  • linear_model/coordinate_descent.py:class LassoCV(LinearModelCV, RegressorMixin):
  • linear_model/coordinate_descent.py:class ElasticNetCV(LinearModelCV, RegressorMixin):
  • linear_model/coordinate_descent.py:class MultiTaskElasticNetCV(LinearModelCV, RegressorMixin):
  • linear_model/coordinate_descent.py:class MultiTaskLassoCV(LinearModelCV, RegressorMixin):
  • linear_model/least_angle.py:class LarsCV(Lars):
  • linear_model/least_angle.py:class LassoLarsCV(LarsCV):
  • linear_model/logistic.py:class LogisticRegressionCV(LogisticRegression, BaseEstimator,
  • linear_model/omp.py:class OrthogonalMatchingPursuitCV(LinearModel, RegressorMixin):
  • linear_model/ridge.py:class RidgeCV(_BaseRidgeCV, RegressorMixin):
  • linear_model/ridge.py:class RidgeClassifierCV(LinearClassifierMixin, _BaseRidgeCV):

@amueller @jnothman @MechCoder @vene @GaelVaroquaux @agramfort
@GaelVaroquaux
Copy link
Member

  1. Should the parameter still be called results_ and not search_results_

+1 for search_results_ in all classes.

@jnothman
Copy link
Member

I have no idea what your 1. means.

I don't feel search_ adds anything in GridSearchCV just as we call something coef_ rather than regression_coef_.

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Aug 19, 2016 via email

@raghavrv
Copy link
Member Author

raghavrv commented Aug 19, 2016 •
edited
Loading

I don't feel search_ adds anything in GridSearchCV just as we call something coef_ rather than regression_coef_.

This issue is for any EstimatorCV other than GridSearchCV and RandomSearchCV...

A results_ attribute in say GradientBoostingClassifierCV might be not super informative about what the attribute holds... (But I don't have a strong opinion.) Let me know your final opinion. Also ping @amueller @vene @MechCoder

Anyway we need to have this (results_ or search_results_) for all EstimatorCVs. Do we agree on that @jnothman?

@raghavrv
Copy link
Member Author

I have no idea what your 1. means.

I stupidly thought we could have abc.abstractproperty for the results_ etc... Nevermind that ;P

@jnothman
Copy link
Member

jnothman commented Aug 20, 2016 •
edited by TomDLT
Loading

cv_results_ any better?

@raghavrv
Copy link
Member Author

raghavrv commented Aug 20, 2016 •
edited
Loading

cv_results_ any better?

I thought about this too and felt it would be much better. Not sure why I didn't suggest it here! I'll go ahead and make this change. Thx

@amueller
Copy link
Member

so cv_results_ in GridSearchCV, RandomSearchCV and EstimatorCVs? I'm ok with that.

@raghavrv
Copy link
Member Author

Could you also share your opinion on #7243 and #7071 (comment)

@renewang
Copy link

renewang commented Aug 1, 2017

Hi, I'm interested in working on this problem. But starting from CalibratedClassifierCV(BaseEstimator, ClassifierMixin), I just wonder, what should be put into cv_results_ for CalibratedClassifierCV because all it does is fitting base_estimator with training set and fitting CalibratedClassifier with testing set. There's no scoring passed in. I'm not sure if this class is fully conform to the BaseSearchCV interface which provides several accessors to the cv_results

@jnothman
Copy link
Member

jnothman commented Aug 1, 2017 via email

@scikit-learn scikit-learn deleted a comment from ffffaaasl Oct 27, 2020
@lucyleeow
Copy link
Member

lucyleeow commented Apr 23, 2024 •
edited
Loading

  • RidgeClassifierCV and RidgeCV use cv_values_
  • GraphicalLassoCV, GraphicalLassoCV, RFECV, HalvingGridSearchCV, HalvingRandomSearchCV, GridSearchCV, RandomSearchCV use cv_results_

There could be mroe consistency here, maybe RidgeClassifierCV and RidgeCV could use cv_results_ instead? Or this issue can be closed?

Other estimator CVs don't have cv results

cc maybe @thomasjpfan ?

@adrinjalali
Copy link
Member

I think deprecating cv_values_ and using cv_results_ everywhere makes sense.

@thomasjpfan
Copy link
Member

I'm okay with deprecating RidgeClassifierCV.cv_values_ and RidgeCV.cv_values_ and going with cv_results_.

@lucyleeow lucyleeow mentioned this issue Apr 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Deprecate cv_values_ in favour of cv_results_ in RidgeCV estimators lucyleeow/scikit-learn
8 participants
@jnothman @GaelVaroquaux @amueller @adrinjalali @renewang @thomasjpfan @raghavrv @lucyleeow