Add random_state parameter to AffinityPropagation#14337
Add random_state parameter to AffinityPropagation#14337rcwoolston wants to merge 7 commits intoscikit-learn:masterfrom
Conversation
|
Thanks for the PR. looks good. Can you please add a test checking that this has an effect, and also covering the error message? |
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks for the PR. Just a few comments
| else (np.array([0]), np.array([0] * n_samples))) | ||
|
|
||
| random_state = np.random.RandomState(0) | ||
| if random_state is None: |
There was a problem hiding this comment.
We have a util for that: utils.check_random_state
There was a problem hiding this comment.
If I am reading the docs right, my if statement should be replaced with utils.check_random_state. @amueller, if that is the case, then do you still want unittests wrapping this function, since they should be caught with unittests from utils.check_random_state? I am good ok with writing those tests, I just want to make sure I following the testing conventions you all use.
There was a problem hiding this comment.
You used check_random_state correctly @rcwoolston 👍
There is no need to check the error message anymore, however a small test making sure that changing the random_state has an effect on the results would still be helpful.
You have some line-too-long issues as can be seen on the ci/circleci: lint checker. You can also check yourself with make flake8-diff
| return_n_iter : bool, default False | ||
| Whether or not to return the number of iterations. | ||
|
|
||
| random_state : int, RandomState instance or None, optional (default=None) |
There was a problem hiding this comment.
For the details we can just link to the glossary. E.g.:
random_state : int, np.random.RandomStateInstance or None, \
optional (default=None)
Pseudo-random number generator to control XXXX. See :term:`random_state`.
… been working in the past
…into affinity_prop_seed
rth
left a comment
There was a problem hiding this comment.
We should also add this parameter to the AffinityPropagation class.
Also please add an entry to the change log at doc/whats_new/v0.22.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself with :user:.
Added random_state parameter to allow user to select the seed.
Relates to issue #14302