Skip to content

[MRG+2] ENH/MNT results_ --> cv_results; test_mean_score --> mean_test_score et al. #7324

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
Merged
merged 1 commit into from
Sep 7, 2016
Merged

[MRG+2] ENH/MNT results_ --> cv_results; test_mean_score --> mean_test_score et al. #7324

merged 1 commit into from
Sep 7, 2016

Conversation

raghavrv
Copy link
Member

@raghavrv raghavrv commented Sep 1, 2016

@raghavrv raghavrv mentioned this pull request Sep 1, 2016
Merged
4 tasks
jnothman
jnothman reviewed Sep 2, 2016
View reviewed changes
doc/whats_new.rst
@@ -40,27 +40,27 @@ Model Selection Enhancements and API Changes
:class:`model_selection.GridSearchCV` and
:class:`model_selection.RandomizedSearchCV` utilities.

- **The enhanced `results_` attribute**
- **The enhanced `cv_results_` attribute**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SHould use double-backticks

@jnothman
Copy link
Member

jnothman commented Sep 2, 2016

What about examples? You should be able to get no results from git grep -w results_ I'd think...

@raghavrv
Copy link
Member Author

raghavrv commented Sep 2, 2016

What about examples?

Oops! Good catch! I think that's why circle is failing...

yenchenlin
yenchenlin reviewed Sep 3, 2016
View reviewed changes
sklearn/model_selection/_search.py
self.cv_results_['params'],
self.cv_results_['mean_test_score'],
self.cv_results_['std_test_score'])):
scores = np.array(list(self.cv_results_['split%d_test_score' %
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe move % to the next line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Thx!

@raghavrv
Copy link
Member Author

raghavrv commented Sep 6, 2016

@jnothman @amueller bump

@jnothman
Copy link
Member

jnothman commented Sep 6, 2016

You didn't actually run that git grep, did you?

@jnothman
Copy link
Member

jnothman commented Sep 6, 2016

Once you've got those last two, the file list covers #6697.

jnothman
jnothman reviewed Sep 6, 2016
View reviewed changes
sklearn/model_selection/_search.py
'test_mean_score' : [0.81, 0.60, 0.75, 0.82],
'test_std_score' : [0.02, 0.01, 0.03, 0.03],
'test_rank_score' : [2, 4, 3, 1],
'split0_test_score' : [0.8, 0.7, 0.8, 0.9],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming with @amueller: these names work better for you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think that's better

@jnothman
Copy link
Member

jnothman commented Sep 6, 2016

And after those tutorial examples, this LGTM.

@jnothman
Copy link
Member

jnothman commented Sep 6, 2016

why cosmit?

@jnothman jnothman changed the title [MRG] COSMIT results_ --> cv_results; test_mean_score --> mean_test_score et al. [MRG+1] COSMIT results_ --> cv_results; test_mean_score --> mean_test_score et al. Sep 6, 2016
@raghavrv raghavrv changed the title [MRG+1] COSMIT results_ --> cv_results; test_mean_score --> mean_test_score et al. [MRG+1] ENH/MNT results_ --> cv_results; test_mean_score --> mean_test_score et al. Sep 6, 2016
@raghavrv
Copy link
Member Author

raghavrv commented Sep 6, 2016

Thanks for the review and +1 :)

@raghavrv
Copy link
Member Author

raghavrv commented Sep 6, 2016

You didn't actually run that git grep, did you?

😛

raghavrv
raghavrv reviewed Sep 6, 2016
View reviewed changes
examples/model_selection/grid_search_digits.py
for i in range(len(clf.results_['params'])):
means = clf.cv_results_['mean_test_score']
stds = clf.cv_results_['std_test_score']
for mean, std, params in zip(means, stds, clf.cv_results_['params']):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this chance to address an old review ;)

@amueller
Copy link
Member

amueller commented Sep 6, 2016

Why did travis not run on this? other than that: LGTM. And changing the book again lol

@amueller amueller changed the title [MRG+1] ENH/MNT results_ --> cv_results; test_mean_score --> mean_test_score et al. [MRG+2] ENH/MNT results_ --> cv_results; test_mean_score --> mean_test_score et al. Sep 6, 2016
@raghavrv
Copy link
Member Author

raghavrv commented Sep 6, 2016

haha

@raghavrv
Copy link
Member Author

raghavrv commented Sep 6, 2016

I'll try a rebase-force-push and see if that restarts travis...

@NelleV
Copy link
Member

NelleV commented Sep 7, 2016

Thanks!

@NelleV NelleV merged commit 35bb1c6 into scikit-learn:master Sep 7, 2016
@raghavrv raghavrv deleted the rename_results_ branch September 7, 2016 19:53
@raghavrv
Copy link
Member Author

raghavrv commented Sep 7, 2016

Thanks for the reviews and merge!

@jnothman
Copy link
Member

jnothman commented Sep 7, 2016

Thank you for doing all the messy work!

On 8 September 2016 at 05:53, Raghav RV notifications@github.com wrote:

Thanks for the reviews and merge!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@raghavrv @jnothman @amueller @NelleV @yenchenlin