Skip to content
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.

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

MRG Training Score in Gridsearch #1742

Closed
wants to merge 1 commit into from

Conversation

amueller
Copy link
Member

@amueller amueller commented Mar 5, 2013

This PR adds training scores to the GridSearchCV output, as wished for by @ogrisel.

'cv_validation_scores'))
CVScoreTuple = namedtuple('CVScoreTuple',
('parameters', 'mean_test_score',
'mean_training_score', 'cv_test_scores'))
Copy link
Member

Choose a reason for hiding this comment

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

Why did you rename *_validation_score to *_test_score? validation sounds more correct in a CV setting. Don't you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

First I thought training and test build a nicer pair. Then I though validation would be better but didn't change it back. Will do once my slides are done ;)

Copy link
Member

Choose a reason for hiding this comment

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

Alright as you wish I don't have any strong opinion on this either.

@ogrisel
Copy link
Member

ogrisel commented Mar 6, 2013

What about measuring the training_duration and testing_duration as well? That should be cheap without any overhead.

@amueller
Copy link
Member Author

amueller commented Mar 6, 2013

It's on the todo. Is there a better way than using time.time?

@ogrisel
Copy link
Member

ogrisel commented Mar 6, 2013

I think time.time is good enough for a start. I don't see any better way.

@amueller
Copy link
Member Author

amueller commented Mar 9, 2013

Fixed doctests, rebased squashed. Should be good to go.

for i, (arr, title) in enumerate(zip(arrays, titles)):
pl.subplot(2, 2, i + 1)
arr = np.array(arr).reshape(len(C_range), len(gamma_range))
#pl.subplots_adjust(left=0.05, right=0.95, bottom=0.15, top=0.95)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a left-over of some experiment? It should be removed if it's not useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. Actually I still need to have a look how it renders on the website.

@ogrisel
Copy link
Member

ogrisel commented Mar 11, 2013

Please add some smoke tests for the new tuple items: for instance check that all of them are positive and that train_score is lower than 1.0.

@ogrisel
Copy link
Member

ogrisel commented Mar 11, 2013

Other than the above comments this looks good to me.

@amueller
Copy link
Member Author

Also added some tests.


We can observe that the lower right half of the parameters (below the diagonal
with high C and gamma values) is characteristic of parameters that yields an
overfitting model: the trainin score is very high but there is a wide gap. The
Copy link
Member

Choose a reason for hiding this comment

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

typo: trainin (my fault)

Copy link
Member

Choose a reason for hiding this comment

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

" wide gap ... with the validation score"

@jnothman
Copy link
Member

See an alternative patch at https://github.com/jnothman/scikit-learn/tree/grid_search_more_info

Note I have chosen different field names, aiming for consistency and memorability, if not preciseness of name.

@amueller
Copy link
Member Author

@jnothman btw, does your version work with lists of dicts as param_grid and with RandomizedSearchCV?
Thinking about it a bit more, I'm not sure your interface is better if the parameter space has a more complicated form. Could you maybe issue a PR? That would make tracking the changes easier.

@jnothman
Copy link
Member

I don't think it's better, but it's certainly no worse: it provides exactly the same ordering according to parameter_iterator as your solution did. If that ordering is meaningful, then the data can be reshaped! If it is not, then you've lost nothing.

It doesn't do anything particular to GridSearchCV, though I see now why you might not want to call the attribute grid_results_. But params_results_ is not nice; point_results_ might work, but fit_grid_point actually fits one fold, not one point.

PR forthcoming.

@amueller
Copy link
Member Author

On 03/13/2013 01:07 AM, jnothman wrote:

I don't think it's better, but it's certainly no worse: it provides
exactly the same ordering according to |parameter_iterator| as your
solution did. If that ordering is meaningful, then the data can be
reshaped! If it is not, then you've lost nothing.

It doesn't do anything particular to |GridSearchCV|, though I see now
why you might not want to call the attribute |grid_results_|. But
|params_results_| is not nice; |point_results_| might work, but
|fit_grid_point| actually fits one fold, not one point.

I know, there are many inconsistencies in the naming.
Both the randomized search and the scoring parameter have been
introduced recently and there is still some polishing to do.
Thanks for all your input!

add docstring for GridSearchCV, RandomizedSearchCV and fit_grid_point. In "fit_grid_point" I used test_score rather than validation_score, as the split is given to the function.
rbf svm grid search example now also shows training scores - which illustrates overfitting for high C, and training/prediction times... which pasically serve to illustrate that this is possible. Maybe random forests would be better to evaluate training times?
@amueller
Copy link
Member Author

superseeded by #7026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants