[MRG] Deprecate random_state in OneClassSVM and add clarifications in docstrings and doc #9703
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ntation of SMO is not random and OneClassSVM does not provide probability estimates
… SVC, LinearSVC and OneClassSVM in the doc
agramfort
reviewed
Sep 7, 2017
sklearn/svm/classes.py
Outdated
| generator; If RandomState instance, random_state is the random number | ||
| generator; If None, the random number generator is the RandomState | ||
| instance used by `np.random`. | ||
| the data for the dual coordinate descent (if ``dual=True``). If int, |
There was a problem hiding this comment.
it is not used with dual=False?
There was a problem hiding this comment.
ok I saw the sentence above. Please add here too:
When ``dual=False`` the underlying implementation is
not random and ``random_state`` has no effect on the results.
agramfort
reviewed
Sep 7, 2017
doc/modules/svm.rst
Outdated
| ``probability`` is set to ``True``). This randomness can be controlled | ||
| with the ``random_state`` parameter. If ``probability`` is set to ``False`` | ||
| these estimators are not random and ``random_state`` has no effect on the | ||
| results. The underlying :class:`OneClassSVM` implementation is similar to |
There was a problem hiding this comment.
two spaces before "The underlying..."
|
besides LGTM |
jnothman
reviewed
Sep 7, 2017
sklearn/svm/classes.py
Outdated
| @@ -1025,6 +1026,10 @@ class OneClassSVM(BaseLibSVM): | |||
| generator; If None, the random number generator is the RandomState | |||
There was a problem hiding this comment.
I think this docstring should be changed to ignored. Unlike other deprecated parameters, it actually has no effect.
|
thanks for the reviews and comments @agramfort and @jnothman |
|
I suppose you'd better add an entry to what's new. |
massich
pushed a commit
to massich/scikit-learn
that referenced
this pull request
Sep 15, 2017
… docstrings and doc (scikit-learn#9703)
maskani-moh
pushed a commit
to maskani-moh/scikit-learn
that referenced
this pull request
Nov 15, 2017
… docstrings and doc (scikit-learn#9703)
jwjohnson314
pushed a commit
to jwjohnson314/scikit-learn
that referenced
this pull request
Dec 18, 2017
… docstrings and doc (scikit-learn#9703)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issue
Fixes issue #9497
What does this implement/fix? Explain your changes.
This deprecates random_state in OneClassSVM as OneClassSVM is not random and does not provide probability estimation. This also clarifies the docstrings of random_state in SVC, NuSVC and LinearSVC and adds information about the randomness of the underlying implementations of these estimators in the User Guide.