API Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py #20161

wowry · 2021-05-28T10:02:54Z

Reference Issues/PRs

Continues #16961
Partially fixes #11198
Based on #16392

What does this implement/fix? Explain your changes.

This PR replaces grid_scores_ by cv_results_ in _rfy.py and test_rfe.py.

Merging changes from the main repository

…rfe.py-update

Rfe.py update

…ps://github.com/arka204/scikit-learn into Replacing-grid_scores_-by-cv_results-in-_rfe.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

sklearn/feature_selection/_rfe.py

ogrisel

LGTM. Just a few minor remarks below (ignore the third that I just left for other reviewers).

sklearn/feature_selection/tests/test_rfe.py

ogrisel · 2021-06-01T17:31:06Z

sklearn/feature_selection/_rfe.py

+        scores_rev = scores[:, ::-1]
+        self.cv_results_ = {}
+        self.cv_results_["mean_score"] = np.mean(scores_rev, axis=0)
+        self.cv_results_["std_score"] = np.std(scores_rev, axis=0)


I am wondering if we should not use mean_test_score / std_test_score / split{i}_test_score instead.

this would be more consistent with GridSearchCV

this would be more future proof in case we want to add an option to also compute the training score in RFECV although this is probably a case of YAGNI.

Edit actuall ignore that remark. #16392 already uses the mean_score notation...

Being consistent with GridSearchCV looks like a net win to me. I am okay with deprecating mean_score in GraphicalLassoCV and switch to using *_test_score instead.

@ogrisel Do you think deprecating would be too much of a hassle?

I am fine with both approaches.

ogrisel · 2021-06-01T17:35:31Z

We probably need an entry to document the deprecation attribute in doc/whats_new/v1.0.rst.

…g-grid_scores_-by-cv_results-in-_rfe.py

wowry · 2021-06-26T07:25:38Z

The black code style has been applied.

ogrisel

I am still fine with this PR but I think you would get a faster second reviewer approval if you address this comment: https://github.com/scikit-learn/scikit-learn/pull/20161/files#r646027286

The handling of the deprecation cycle of the same attributes in GraphicalLassoCV can be tackled in a separate PR.

sklearn/feature_selection/_rfe.py

thomasjpfan · 2021-07-22T18:59:38Z

I am +1 on changing the names of the keys as described in #20161 (comment)

Would it be okay to make this change in this PR?

wowry · 2021-07-22T22:15:38Z

@thomasjpfan
Certainly. I appreciate your help.

glemaitre · 2021-07-27T17:12:32Z

sklearn/feature_selection/_rfe.py

+        self.cv_results_["mean_score"] = np.mean(scores_rev, axis=0)
+        self.cv_results_["std_score"] = np.std(scores_rev, axis=0)


I think that @thomasjpfan mentioned in its previous post was to change both entries using mean_test_score and std_test_score here.

I'm sorry, I was mistaken about that.
Would it be okay to just change the names?

yes it should be enough

I understand. Thank you so much.

…es_-by-cv_results-in-_rfe.py

sklearn/feature_selection/_rfe.py

sklearn/feature_selection/tests/test_rfe.py

glemaitre · 2021-07-30T15:45:25Z

LGTM. I pushed a tiny nitpick for the f-string styling.
I will merge once the CIs are green.

wowry · 2021-07-30T15:48:41Z

Thank you for all of you.

…cikit-learn#20161) Co-authored-by: arka204 <kmichalik204@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

scikit-learn/scikit-learn#20161

arka204 and others added 18 commits April 11, 2020 18:47

Merge pull request scikit-learn#1 from scikit-learn/master

3b79637

Merging changes from the main repository

Deprecating grid_scores_ in _rfe.py.

43f64a0

Merge pull request scikit-learn#2 from scikit-learn/master

464dc37

Merging changes from the main repository

Merge pull request scikit-learn#3 from scikit-learn/master

15a658f

Merging changes from the main repository

Fixing linting errors.

672bca7

Modifying tests and fixing mypy error.

9448514

Merge branch 'Replacing-grid_scores_-by-cv_results-in-_rfe.py' into _…

fc36365

…rfe.py-update

Merge pull request scikit-learn#7 from arka204/_rfe.py-update

68436b2

Rfe.py update

Fixing small errors.

41bc337

Merge branch 'Replacing-grid_scores_-by-cv_results-in-_rfe.py' of htt…

1914f5e

…ps://github.com/arka204/scikit-learn into Replacing-grid_scores_-by-cv_results-in-_rfe.py

Applying suggestions.

2beb458

Applaying suggestions.

1712464

Update sklearn/feature_selection/tests/test_rfe.py

ec05856

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Fix merge conflicts

24835e2

Update deprecation messages of grid_scores_

f781f30

Fix computation of cv_results_ in _rfe.py

b4ae6f4

Fix tests of cv_results_ in test_rfe.py

d8ce1c4

Fix documentation in _rfe.py

45e6ffa

github-actions bot added the module:feature_selection label May 28, 2021

wowry mentioned this pull request May 28, 2021

[MRG] Replacing grid_scores_ by cv_results_ in _rfe.py #16961

Closed

wowry changed the title ~~Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py~~ [WIP] Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py May 28, 2021

wowry changed the title ~~[WIP] Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py~~ [MRG] Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py May 28, 2021

ogrisel reviewed May 28, 2021

View reviewed changes

sklearn/feature_selection/_rfe.py Outdated Show resolved Hide resolved

wowry added 2 commits May 28, 2021 23:14

Merge branch 'main' into Replacing-grid_scores_-by-cv_results-in-_rfe.py

4f23ec4

Update deprecation messages of grid_scores_

42ecf9d

wowry requested a review from ogrisel May 29, 2021 21:36

ogrisel approved these changes Jun 1, 2021

View reviewed changes

ogrisel added the Waiting for Reviewer label Jun 1, 2021

Update deprecation of grid_scores_

0e820e0

wowry requested a review from ogrisel June 12, 2021 09:19

wowry and others added 4 commits June 25, 2021 23:30

Merge commit '0e7761cdc4f244adb4803f1a97f0a9fe4b365a99' into Replacin…

7c786a9

…g-grid_scores_-by-cv_results-in-_rfe.py

MAINT Adds target_version to black config (scikit-learn#20293)

63a1bfa

Apply black

20d733a

Merge branch 'main' into Replacing-grid_scores_-by-cv_results-in-_rfe.py

b70baa9

wowry requested a review from thomasjpfan June 30, 2021 03:59

ogrisel approved these changes Jul 12, 2021

View reviewed changes

This was referenced Jul 20, 2021

Deprecate grid_scores_ everywhere, replace by cv_results_ #11198

Closed

API Deprecates mean_score, std_score, split(k)_score in GraphicalLassoCV #20583

Merged

glemaitre self-requested a review July 22, 2021 16:32

Merge remote-tracking branch 'origin/main' into pr/wowry/20161

b3ae926

glemaitre reviewed Jul 22, 2021

View reviewed changes

sklearn/feature_selection/_rfe.py Outdated Show resolved Hide resolved

wowry and others added 2 commits July 23, 2021 07:59

Fix deprecation messages

007d427

Merge branch 'main' into Replacing-grid_scores_-by-cv_results-in-_rfe.py

3a40617

glemaitre reviewed Jul 27, 2021

View reviewed changes

wowry added 2 commits July 30, 2021 23:03

Merge remote-tracking branch 'upstream/main' into Replacing-grid_scor…

0be435a

…es_-by-cv_results-in-_rfe.py

Change names of the keys into '*_test_score'

4247091

glemaitre approved these changes Jul 30, 2021

View reviewed changes

sklearn/feature_selection/_rfe.py Outdated Show resolved Hide resolved

sklearn/feature_selection/tests/test_rfe.py Outdated Show resolved Hide resolved

Apply suggestions from code review

bd94dd4

glemaitre merged commit daec880 into scikit-learn:main Jul 30, 2021

wowry deleted the Replacing-grid_scores_-by-cv_results-in-_rfe.py branch July 30, 2021 16:16

eddiebergman mentioned this pull request Nov 15, 2022

Update scikit learn 1.2 automl/auto-sklearn#1611

Closed

54 tasks

gregcaporaso added a commit to qiime2/q2-sample-classifier that referenced this pull request Apr 30, 2024

deprecated grid_scores_ to cv_results_

6a7e6df

scikit-learn/scikit-learn#20161

gregcaporaso added a commit to qiime2/q2-sample-classifier that referenced this pull request May 1, 2024

deprecated grid_scores_ to cv_results_

eb6d6cb

scikit-learn/scikit-learn#20161

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py #20161

API Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py #20161

wowry commented May 28, 2021

ogrisel left a comment

ogrisel Jun 1, 2021

thomasjpfan Jun 5, 2021

ogrisel Jun 24, 2021

ogrisel commented Jun 1, 2021

wowry commented Jun 26, 2021

ogrisel left a comment •

edited

thomasjpfan commented Jul 22, 2021

wowry commented Jul 22, 2021

glemaitre Jul 27, 2021

wowry Jul 30, 2021

glemaitre Jul 30, 2021

wowry Jul 30, 2021

glemaitre commented Jul 30, 2021

wowry commented Jul 30, 2021

		self.cv_results_["mean_score"] = np.mean(scores_rev, axis=0)
		self.cv_results_["std_score"] = np.std(scores_rev, axis=0)

API Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py #20161

API Replacing grid_scores_ by cv_results_ in _rfe.py and test_rfe.py #20161

Conversation

wowry commented May 28, 2021

Reference Issues/PRs

What does this implement/fix? Explain your changes.

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel Jun 1, 2021

Choose a reason for hiding this comment

thomasjpfan Jun 5, 2021

Choose a reason for hiding this comment

ogrisel Jun 24, 2021

Choose a reason for hiding this comment

ogrisel commented Jun 1, 2021

wowry commented Jun 26, 2021

ogrisel left a comment • edited

Choose a reason for hiding this comment

thomasjpfan commented Jul 22, 2021

wowry commented Jul 22, 2021

glemaitre Jul 27, 2021

Choose a reason for hiding this comment

wowry Jul 30, 2021

Choose a reason for hiding this comment

glemaitre Jul 30, 2021

Choose a reason for hiding this comment

wowry Jul 30, 2021

Choose a reason for hiding this comment

glemaitre commented Jul 30, 2021

wowry commented Jul 30, 2021

ogrisel left a comment •

edited