Skip to content

[MRG + 1] Printing the total time in cross_validation#7640

Merged
TomDLT merged 3 commits intoscikit-learn:masterfrom
srivatsan-ramesh:dev
Oct 24, 2016
Merged

[MRG + 1] Printing the total time in cross_validation#7640
TomDLT merged 3 commits intoscikit-learn:masterfrom
srivatsan-ramesh:dev

Conversation

@srivatsan-ramesh
Copy link
Copy Markdown
Contributor

@srivatsan-ramesh srivatsan-ramesh commented Oct 11, 2016

Reference Issue

Fixes #7639

What does this implement/fix? Explain your changes.

Initially when doing cross_validation with verbose set, the code was printing the score_time instead of the score_time + fit_time. This PR fixes that and prints the total time.

Any other comments?

@jnothman if you could elaborate on the test which you said has to be done in the comments of the issue #7639 I can add that also to this PR.

raghavrv
raghavrv reviewed Oct 11, 2016
View reviewed changes
sklearn/model_selection/_validation.py Outdated
if verbose > 1:
end_msg = "%s -%s" % (msg, logger.short_format_time(score_time))
total_time = score_time + fit_time
end_msg = "%s -%s" % (msg, logger.short_format_time(total_time))
Copy link
Copy Markdown
Member

@raghavrv raghavrv Oct 11, 2016

Choose a reason for hiding this comment

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

You could directly substitute it here :)

Copy link
Copy Markdown
Contributor Author

@srivatsan-ramesh srivatsan-ramesh Oct 11, 2016

Choose a reason for hiding this comment

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

Yes, but the line was getting too long (>79). So instead of breaking the line, I did this. So, should I change it again?

Copy link
Copy Markdown
Member

@raghavrv raghavrv Oct 11, 2016

Choose a reason for hiding this comment

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

No it's fine I suppose.

Copy link
Copy Markdown
Member

@amueller amueller Oct 11, 2016

Choose a reason for hiding this comment

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

shouldn't it say total time? or fit time and score time separately? At least total: %s maybe?

Copy link
Copy Markdown
Contributor Author

@srivatsan-ramesh srivatsan-ramesh Oct 12, 2016

Choose a reason for hiding this comment

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

Modified the log message

raghavrv
raghavrv approved these changes Oct 11, 2016
View reviewed changes
sklearn/model_selection/_validation.py Outdated
if verbose > 1:
end_msg = "%s -%s" % (msg, logger.short_format_time(score_time))
total_time = score_time + fit_time
end_msg = "%s -%s" % (msg, logger.short_format_time(total_time))
Copy link
Copy Markdown
Member

@raghavrv raghavrv Oct 11, 2016

Choose a reason for hiding this comment

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

No it's fine I suppose.

@raghavrv
Copy link
Copy Markdown
Member

raghavrv commented Oct 11, 2016

Thanks for the PR!

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 12, 2016

Well we can test correspondence between the verbose output and the results in GridSearchCV. I suppose it wouldn't hurt to do that.

@srivatsan-ramesh
Copy link
Copy Markdown
Contributor Author

srivatsan-ramesh commented Oct 13, 2016

@jnothman a small clarification, should I check if the mean of all the scores printed through verbose setting is equal to the score obtained from GridSearchCV? Is that what you meant?

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 13, 2016

Yes, I suppose you can only check the mean fit time + mean score time.
Maybe we can do without a test here as we don't want a test that takes long
just for the sake of a non-trivial test case...

On 13 October 2016 at 15:47, Srivatsan notifications@github.com wrote:

@jnothman https://github.com/jnothman a small clarification, should I
check if the mean of all the scores printed through verbose setting is
equal to the score obtained from GridSearchCV? Is that what you meant?

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

@srivatsan-ramesh
Copy link
Copy Markdown
Contributor Author

srivatsan-ramesh commented Oct 13, 2016

Ok, So since we don't want a test, this PR is ready for [MRG] right?

@srivatsan-ramesh srivatsan-ramesh changed the title [WIP] Printing the total time in cross_validation [MRG] Printing the total time in cross_validation Oct 13, 2016
@raghavrv raghavrv changed the title [MRG] Printing the total time in cross_validation [MRG + 1] Printing the total time in cross_validation Oct 24, 2016
@raghavrv
Copy link
Copy Markdown
Member

raghavrv commented Oct 24, 2016

@amueller One more review and merge?

@TomDLT
Copy link
Copy Markdown
Member

TomDLT commented Oct 24, 2016

LGTM, merging, thanks again @srivatsan-ramesh

@TomDLT TomDLT merged commit 177ac84 into scikit-learn:master Oct 24, 2016
@raghavrv raghavrv added this to the 0.18.1 milestone Oct 24, 2016
amueller pushed a commit to amueller/scikit-learn that referenced this pull request Oct 25, 2016
@srivatsan-ramesh @amueller
[MRG + 1] Printing the total time in cross_validation (scikit-learn#7640
18e2fc9 
)

* print score+fit time instead of just score time when doing cross_validation

* reducing line size

* More clearer log message
@srivatsan-ramesh srivatsan-ramesh deleted the dev branch October 25, 2016 07:32
sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
@srivatsan-ramesh @sergeyf
[MRG + 1] Printing the total time in cross_validation (scikit-learn#7640
dbe48cd 
)

* print score+fit time instead of just score time when doing cross_validation

* reducing line size

* More clearer log message
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@srivatsan-ramesh @Sundrique
[MRG + 1] Printing the total time in cross_validation (scikit-learn#7640
e415d16 
)

* print score+fit time instead of just score time when doing cross_validation

* reducing line size

* More clearer log message
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@srivatsan-ramesh @paulha
[MRG + 1] Printing the total time in cross_validation (scikit-learn#7640
7ffac46 
)

* print score+fit time instead of just score time when doing cross_validation

* reducing line size

* More clearer log message
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@srivatsan-ramesh @maskani-moh
[MRG + 1] Printing the total time in cross_validation (scikit-learn#7640
9ab75a9 
)

* print score+fit time instead of just score time when doing cross_validation

* reducing line size

* More clearer log message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amueller amueller amueller left review comments

+2 more reviewers

@raghavrv raghavrv raghavrv approved these changes

@RPGOne RPGOne RPGOne approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.18.1

Development

Successfully merging this pull request may close these issues.

Timings in crossvalidation

6 participants

@srivatsan-ramesh @raghavrv @jnothman @TomDLT @amueller @RPGOne