Add random_state parameter to AffinityPropagation #14337
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? |
| @@ -133,7 +139,10 @@ def affinity_propagation(S, preference=None, convergence_iter=15, max_iter=200, | |||
| if return_n_iter | |||
| 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
| @@ -72,6 +72,12 @@ def affinity_propagation(S, preference=None, convergence_iter=15, max_iter=200, | |||
| 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
Added random_state parameter to allow user to select the seed.
Relates to issue #14302