-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Added user guide documentation for permutation_test_score #10905 #14769
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR,
Only nitpicks for now, I'll look at it into more details later but it looks good.
Doc building is failing on the CI, not sure why yet
@@ -235,6 +235,20 @@ Here is an example of ``cross_validate`` using a single metric:: | |||
>>> sorted(scores.keys()) | |||
['estimator', 'fit_time', 'score_time', 'test_score', 'train_score'] | |||
|
|||
Cross-validation significance evaluation | |||
---------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add this section after "Obtaining predictions by cross-validation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in PR #14757.
@@ -871,7 +871,20 @@ def _index_param_value(X, v, indices): | |||
def permutation_test_score(estimator, X, y, groups=None, cv=None, | |||
n_permutations=100, n_jobs=1, random_state=0, | |||
verbose=0, scoring=None): | |||
"""Evaluate the significance of a cross-validated score with permutations | |||
"""Evaluate the significance of a cross-validated score by permuting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following pep8, we try to keep the first sentence to 1 line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in PR #14757.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing this pull request in favour of #14757
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Fixes #10905. Also see #10931. Worked on the branch https://github.com/maskani-moh/scikit-learn/tree/fix-10905.
Fix the typos from previous pull request by @aditi9783
Any other comments?