Skip to content

MRG: DOC: Simpler cross-validation iterator doc#2370

Merged
ogrisel merged 5 commits intoscikit-learn:masterfrom
ogrisel:doc-cross-validation
Sep 10, 2013
Merged

MRG: DOC: Simpler cross-validation iterator doc#2370
ogrisel merged 5 commits intoscikit-learn:masterfrom
ogrisel:doc-cross-validation

Conversation

@ogrisel
Copy link
Copy Markdown
Member

@ogrisel ogrisel commented Aug 19, 2013

Remove the reference to data arrays in the inline example and be more explicit to explain the difference between LOLO and StratifiedKFold.

amueller
amueller reviewed Aug 20, 2013
View reviewed changes
doc/modules/cross_validation.rst
Copy link
Copy Markdown
Member

@amueller amueller Aug 20, 2013

Choose a reason for hiding this comment

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

how did the doctest work before? the deprecation of indices=False is merged, right?

Copy link
Copy Markdown
Member Author

@ogrisel ogrisel Aug 20, 2013

Choose a reason for hiding this comment

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

Nope.

@amueller
Copy link
Copy Markdown
Member

amueller commented Aug 20, 2013

LGTM apart from the minor nitpicks / remarks. Not sure if showing the resulting iterators is good or not. If you don't feel like playing, I wouldn't mind merging it as it is.

robertlayton
robertlayton reviewed Aug 20, 2013
View reviewed changes
doc/modules/cross_validation.rst Outdated
Copy link
Copy Markdown
Member

@robertlayton robertlayton Aug 20, 2013

Choose a reason for hiding this comment

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

I may be overly nitpicking here, but: similar percentage of samples? It's only the same under certain circumstances.

@robertlayton
Copy link
Copy Markdown
Member

robertlayton commented Aug 20, 2013

Looks good to me +1

@ogrisel
Copy link
Copy Markdown
Member Author

ogrisel commented Aug 21, 2013

I addressed your comments. I would still get people's opinion on removing the not very informative print(cv_object) statements to let the reader focus on the important part of the snippets: the actual cv splitting logic.

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Aug 21, 2013

I would still get people's opinion on removing the not very
informative print(cv_object) statements to let the reader focus on
the important part of the snippets: the actual cv splitting logic.

I think that this is good.

@ogrisel
Copy link
Copy Markdown
Member Author

ogrisel commented Aug 21, 2013

I think that this is good.

You mean, +1 for removal?

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Aug 21, 2013 

 I think that this is good.

You mean, +1 for removal?

Yes

@ogrisel
Copy link
Copy Markdown
Member Author

ogrisel commented Aug 21, 2013

Alright, done.

@ogrisel
Copy link
Copy Markdown
Member Author

ogrisel commented Aug 21, 2013

Any more comments? Shall we merge?

@glouppe
Copy link
Copy Markdown
Contributor

glouppe commented Sep 10, 2013

This looks good to me. I also agree with the removal of the print(cv_objects). The documentation is now clearer.

+1 for merge

ogrisel added a commit that referenced this pull request Sep 10, 2013
@ogrisel
Merge pull request #2370 from ogrisel/doc-cross-validation
f128072 
MRG: DOC: Simpler cross-validation iterator doc
@ogrisel ogrisel merged commit f128072 into scikit-learn:master Sep 10, 2013
@ogrisel
Copy link
Copy Markdown
Member Author

ogrisel commented Sep 10, 2013

Thanks!

@ogrisel ogrisel mentioned this pull request Sep 10, 2013
Closed
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

@ogrisel @amueller @robertlayton @GaelVaroquaux @glouppe