[MRG] Add random_state parameter to AffinityPropagation by cmarmo · Pull Request #16801 · scikit-learn/scikit-learn

cmarmo · 2020-03-29T16:46:42Z

Reference Issues/PRs

Continues and closes #14337.

What does this implement/fix? Explain your changes.

Addresses comments in #14337:

add parameter to AffinityPropagation class,
add test,
add what's new entry.

Any other comments?

This is the last open PR from the 2019 Scipy Sprint.

…nted at glossery

… been working in the past

…into affinity_prop_seed

adrinjalali

Thanks @cmarmo

sklearn/cluster/_affinity_propagation.py

Co-Authored-By: Adrin Jalali <adrin.jalali@gmail.com>

adrinjalali · 2020-03-30T10:56:03Z

Whenever we change the behavior of a mode/method/etc, we tend to deprecate the old behavior away, instead of changing it immediately. The exceptions are bug fixes, which we just fix them and as a result there's a change in the model's behavior.

In this case, the model's behavior has been that it's deterministic with a random seed of 0. We're changing the behavior now to be non-deterministic when the default random_state is None. In a way, we can think of it as changing the default value of random_state from 0 to None.

I'd also be happy if we think this is simply a bug fix and therefore doesn't require deprecation. We can wait for another opinion on this one :)

NicolasHug

Thanks @cmarmo , looks good.

I agree the backward compat is an issue. I'm not sure we can make it a bugfix, that'd be pushing it IMHO. One option is to default to e.g. random_state='warn' now which would be equivalent to 0 and which would raise a warning telling users to set it manually, and switch to None is 2 versions. But that's a bit annoying for them because we're warning on perfectly fine code.

sklearn/cluster/_affinity_propagation.py

doc/whats_new/v0.23.rst

sklearn/cluster/tests/test_affinity_propagation.py

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

sklearn/cluster/tests/test_affinity_propagation.py

cmarmo · 2020-04-01T15:34:12Z

🎉

sklearn/cluster/tests/test_affinity_propagation.py

sklearn/tests/test_docstring_parameters.py

…tes.

cmarmo · 2020-04-01T19:11:19Z

The failing test is in master.

thomasjpfan

LGTM

cmarmo · 2020-05-05T10:33:06Z

@adrinjalali I'm wondering if this one could still be considered for 0.23... thanks!

NicolasHug · 2020-05-05T11:55:42Z

The current solution now throws a warning for perfectly fine code and that's not ideal. I don't know what others think?
Maybe you'll want to discuss this during the next meeting @cmarmo .

adrinjalali

Otherwise LGTM. I'm not worried about the warning by default, since it's the same in many other cases when we deprecate or change the behavior of a parameter, so there's nothing new there.

adrinjalali · 2020-05-05T12:09:03Z

sklearn/cluster/_affinity_propagation.py

+                       "call. To silence this warning and obtain the same "
+                       "results as before 0.23, set it to 0."),


I would suggest this to recommend users to set it explicitly to None rather than 0. Maybe something like:

"Set random_state to None to silence this warning, or to 0 to keep the deprecated behavior."

deprecated -> previous

there's no deprecated behaviour

Sure, I don't mind. I consider the default value 0 as the random seed deprecated, but don't really mind.

NicolasHug · 2020-05-05T12:22:10Z

so there's nothing new there.

I personally don't recall doing something like this in the past. We usually change the default behaviour of an existing parameter, or we rename it so as to get a more sensible name.

But that's different here: we're adding a new parameter and we cannot set its default in a backward compatible way. I would be more comfortable with additional opinions.

adrinjalali · 2020-05-05T12:27:07Z

@rth WDYT?

rth · 2020-05-05T12:34:21Z

The current solution now throws a warning for perfectly fine code and that's not ideal. I don't know what others think?

You mean that it raises a warning with default parameters? As soon as the user provides any random state be it 0 or None, there should be no warning, right?

+1 For the #16801 (comment) otherwise I don't really see what we could do better and the current approach sounds reasonable to me.

adrinjalali · 2020-05-05T13:26:05Z

Shall we merge then @NicolasHug ?

adrinjalali · 2020-05-05T14:40:19Z

Merging with 3 approvals, happy to chat about it in our call :)

…6801) * Added value checks and random state parameter to method * Changed default parameter to None instead of 0 * Added numpy RandomState to the check * Replaced inline validation with check_random_state from utils and pointed at glossery * Needed a different default parameter to pass the default way this has been working in the past * Updated to conform with flake8 stds * Add random_state to AffinityPropagation class. * Add test. * Add what's new entry and versionadded directive. * Add PR number. * Fix lint error due to this PR. * Use np.array_equal in test. * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Adrin Jalali <adrin.jalali@gmail.com> * Homogenize parametre descriptions, default random_state to None. * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update doc/whats_new/v0.23.rst Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Change test name. * Modify check in test. * Fix lint errors. * Address comment. * Address comment. * Add 'deprecation' and its correspondent test. * Fix lint errors. * Add random_state parameter to tests, to avoid FutureWarnings. * Move warning in fit. Modify tests. * Modify example. * Tentative fix for failures. * Document default value to 0. Revert docstring. * Explicit link to Glossary. * Fix default value. * Remove some warnings from tests. * Validate and test docstring. * Tentative fix. * Tentative fix. * Ignore FutureWarning in fit attribute test. * Set random_state to avoid FutureWarning in test_fit_docstring_attributes. * [doc build] Force documentation build. * Clarify warning message. Co-authored-by: rwoolston.admin <rwoolston.admin@LXO-DS-DEV.afcucorp.local> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

* Added value checks and random state parameter to method * Changed default parameter to None instead of 0 * Added numpy RandomState to the check * Replaced inline validation with check_random_state from utils and pointed at glossery * Needed a different default parameter to pass the default way this has been working in the past * Updated to conform with flake8 stds * Add random_state to AffinityPropagation class. * Add test. * Add what's new entry and versionadded directive. * Add PR number. * Fix lint error due to this PR. * Use np.array_equal in test. * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Adrin Jalali <adrin.jalali@gmail.com> * Homogenize parametre descriptions, default random_state to None. * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update doc/whats_new/v0.23.rst Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Change test name. * Modify check in test. * Fix lint errors. * Address comment. * Address comment. * Add 'deprecation' and its correspondent test. * Fix lint errors. * Add random_state parameter to tests, to avoid FutureWarnings. * Move warning in fit. Modify tests. * Modify example. * Tentative fix for failures. * Document default value to 0. Revert docstring. * Explicit link to Glossary. * Fix default value. * Remove some warnings from tests. * Validate and test docstring. * Tentative fix. * Tentative fix. * Ignore FutureWarning in fit attribute test. * Set random_state to avoid FutureWarning in test_fit_docstring_attributes. * [doc build] Force documentation build. * Clarify warning message. Co-authored-by: rwoolston.admin <rwoolston.admin@LXO-DS-DEV.afcucorp.local> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

…6801) * Added value checks and random state parameter to method * Changed default parameter to None instead of 0 * Added numpy RandomState to the check * Replaced inline validation with check_random_state from utils and pointed at glossery * Needed a different default parameter to pass the default way this has been working in the past * Updated to conform with flake8 stds * Add random_state to AffinityPropagation class. * Add test. * Add what's new entry and versionadded directive. * Add PR number. * Fix lint error due to this PR. * Use np.array_equal in test. * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Adrin Jalali <adrin.jalali@gmail.com> * Homogenize parametre descriptions, default random_state to None. * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update sklearn/cluster/_affinity_propagation.py Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Update doc/whats_new/v0.23.rst Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com> * Change test name. * Modify check in test. * Fix lint errors. * Address comment. * Address comment. * Add 'deprecation' and its correspondent test. * Fix lint errors. * Add random_state parameter to tests, to avoid FutureWarnings. * Move warning in fit. Modify tests. * Modify example. * Tentative fix for failures. * Document default value to 0. Revert docstring. * Explicit link to Glossary. * Fix default value. * Remove some warnings from tests. * Validate and test docstring. * Tentative fix. * Tentative fix. * Ignore FutureWarning in fit attribute test. * Set random_state to avoid FutureWarning in test_fit_docstring_attributes. * [doc build] Force documentation build. * Clarify warning message. Co-authored-by: rwoolston.admin <rwoolston.admin@LXO-DS-DEV.afcucorp.local> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

rwoolston.admin and others added 13 commits July 13, 2019 11:51

Added value checks and random state parameter to method

5168c91

Changed default parameter to None instead of 0

ff35479

Added numpy RandomState to the check

3760ea3

Replaced inline validation with check_random_state from utils and poi…

c3d42af

…nted at glossery

Needed a different default parameter to pass the default way this has…

a06bb8a

… been working in the past

Updated to conform with flake8 stds

12c8a38

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

4e7fded

…into affinity_prop_seed

Sync with upstream.

a9039e3

Add random_state to AffinityPropagation class.

91ab164

Add test.

e40b5d4

Add what's new entry and versionadded directive.

8a2ee79

Add PR number.

c28292c

Fix lint error due to this PR.

1837379

github-actions bot added the module:cluster label Mar 29, 2020

Use np.array_equal in test.

9119fe7

adrinjalali reviewed Mar 30, 2020

View reviewed changes

sklearn/cluster/_affinity_propagation.py Outdated Show resolved Hide resolved

sklearn/cluster/_affinity_propagation.py Outdated Show resolved Hide resolved

sklearn/cluster/_affinity_propagation.py Show resolved Hide resolved

cmarmo and others added 2 commits March 30, 2020 10:41

Update sklearn/cluster/_affinity_propagation.py

fa4f484

Co-Authored-By: Adrin Jalali <adrin.jalali@gmail.com>

Homogenize parametre descriptions, default random_state to None.

6ae3d15

NicolasHug reviewed Mar 30, 2020

View reviewed changes

cmarmo and others added 7 commits March 30, 2020 14:04

Update sklearn/cluster/_affinity_propagation.py

837cf51

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

Update sklearn/cluster/_affinity_propagation.py

ac5d2ea

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

Update sklearn/cluster/_affinity_propagation.py

2a83abd

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

Update doc/whats_new/v0.23.rst

540da7f

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

Change test name.

ace190d

Modify check in test.

cd85476

Fix lint errors.

9b94384

NicolasHug reviewed Mar 30, 2020

View reviewed changes

sklearn/cluster/tests/test_affinity_propagation.py Outdated Show resolved Hide resolved

NicolasHug reviewed Mar 30, 2020

View reviewed changes

sklearn/cluster/tests/test_affinity_propagation.py Outdated Show resolved Hide resolved

Address comment.

26e3147

cmarmo added 3 commits April 1, 2020 12:40

Tentative fix.

5c18045

Tentative fix.

7ca015f

Ignore FutureWarning in fit attribute test.

a1fbf60

adrinjalali reviewed Apr 1, 2020

View reviewed changes

sklearn/cluster/tests/test_affinity_propagation.py Show resolved Hide resolved

sklearn/tests/test_docstring_parameters.py Outdated Show resolved Hide resolved

cmarmo added 2 commits April 1, 2020 19:12

Set random_state to avoid FutureWarning in test_fit_docstring_attribu…

d0583ad

…tes.

[doc build] Force documentation build.

bba83f2

cmarmo added 2 commits April 3, 2020 10:59

Merge branch 'master' into affinity_prop_seed

cc0c705

Merge branch 'master' into affinity_prop_seed

20df69f

cmarmo added the Waiting for Reviewer label Apr 9, 2020

Sync with upstream.

0d0eaa4

thomasjpfan approved these changes Apr 28, 2020

View reviewed changes

adrinjalali reviewed May 5, 2020

View reviewed changes

Merge branch 'master' into affinity_prop_seed

46b420f

cmarmo removed the Waiting for Reviewer label May 5, 2020

Clarify warning message.

b3b8d31

adrinjalali merged commit f72d971 into scikit-learn:master May 5, 2020

cmarmo deleted the affinity_prop_seed branch May 5, 2020 16:19

		"call. To silence this warning and obtain the same "
		"results as before 0.23, set it to 0."),

Uh oh!

Conversation

cmarmo commented Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrinjalali commented Mar 30, 2020

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmarmo commented Apr 1, 2020

Uh oh!

Uh oh!

Uh oh!

cmarmo commented Apr 1, 2020

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

cmarmo commented May 5, 2020

Uh oh!

NicolasHug commented May 5, 2020

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali May 5, 2020

Choose a reason for hiding this comment

Uh oh!

NicolasHug May 5, 2020

Choose a reason for hiding this comment

Uh oh!

adrinjalali May 5, 2020

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented May 5, 2020

Uh oh!

adrinjalali commented May 5, 2020

Uh oh!

rth commented May 5, 2020

Uh oh!

adrinjalali commented May 5, 2020

Uh oh!

adrinjalali commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cmarmo commented Mar 29, 2020 •

edited

Loading