[MRG + 1] Remove "warn_on_dtype" from check_array by praths007 · Pull Request #13382 · scikit-learn/scikit-learn

praths007 · 2019-03-04T12:54:09Z

Reference Issues/PRs:

Remove "warn_on_dtype" from check_array #13324

Implemented changes:

Changes made:

deprecated warn_on_dtype from check_array and check_X_y
the only occurrence of a DataConversionWarning is now handled directly in check_pairwise_array since this is only related to boolean metrics.

merge fork

…calculated

merge fork

…https://github.com/praths007/scikit-learn into remove_warn_on_dtype_from_check_array

jnothman

It's very hard to focus on the substance of your pr with all the other changes

sklearn/metrics/pairwise.py

merge fork

sklearn/metrics/pairwise.py

NicolasHug · 2019-03-18T14:53:49Z

@jnothman shouldn't there be a ChangedBehaviourWarning warning in check_pairwise_arrays?

The DataConversionWarning won't be raised anymore regardless of the dtype param of check_pariwise_array.

jnothman · 2019-03-19T06:50:50Z

Overall I'm not yet committed to this change. We had problems with the other use of warn_on_dtype and I'm not sure we gain from removing it here. But do you mean that we should issue a ChangedBehaviorWarning in cases where we used to issue a conversion warning?

…to remove_warn_on_dtype_from_check_array

praths007 · 2019-03-19T09:52:13Z

There are places where test cases break because of raising a DeprecationWarning:

Some of these test cases actually check the number of warnings being raised like test_affinity_propagation_convergence_warning_dense_sparse
Others have assert statements that indirectly check for warnings using assert_no_warnings

So if there is a DeprecationWarning raised it will still break the test case as assert will be wrong. Is there any way around this?

NicolasHug · 2019-03-19T11:15:20Z

But do you mean that we should issue a ChangedBehaviorWarning in cases where we used to issue a conversion warning?

Or even more conservative, go through a deprecation cycle? We're changing the behaviour of a public function, unless it's a bugfix we need a deprecation cycle right?

(I'm not suggesting we should do that, overall I'm just curious about when deprecation cycles are relevant and when they're not)

jnothman · 2019-03-19T22:55:36Z

We're changing a side-effect rather than a behaviour of a public function... I don't think there's any real need to deprecate.

praths007 · 2019-03-22T08:56:10Z

I believe that my limited understanding of this issue causes this PR to not be efficacious. Should I go ahead and close it? Perhaps someone else can provide a better solution.

NicolasHug

Some comments.

@jnothman

Overall I'm not yet committed to this change

So you've changed your mind since #13324 (comment)?

I believe that my limited understanding of this issue causes this PR to not be efficacious

You're almost there @praths007 !

What this PR needs to do is:

deprecate warn_on_dtype from check_array and keep the current behaviour (i.e. warning on dtype conversion).
remove its use from check_pariwise_arrays. No need for a deprecation cycle here.

You've already done most of it (please see my comments below).

Though I would suggest to wait for Joel's feedback before going back to it to avoid unnecessary work...

sklearn/utils/tests/test_validation.py

NicolasHug · 2019-03-22T11:44:41Z

sklearn/utils/tests/test_validation.py


+def test_check_array_warn_on_dtype_deprecation():
+    X = np.asarray([[0.0], [1.0]])
+    assert_warns(DeprecationWarning, check_array, X, warn_on_dtype=True)


Please use pytest.wars with the match parameter as shown here, instead of assert_warns

sklearn/utils/tests/test_validation.py

sklearn/utils/validation.py

NicolasHug

Some comments.

@jnothman

Overall I'm not yet committed to this change

So you've changed your mind since #13324 (comment)?

I believe that my limited understanding of this issue causes this PR to not be efficacious

You're almost there @praths007 !

What this PR needs to do is:

deprecate warn_on_dtype from check_array and keep the current behaviour (i.e. warning on dtype conversion).
remove its use from check_pariwise_arrays. No need for a deprecation cycle here.

You've already done most of it (please see my comments below).

Though I would suggest to wait for Joel's feedback before going back to it to avoid unnecessary work...

jnothman · 2019-03-23T10:26:40Z

I think my opinion then was fairly ambivalent, not a strong vote for its removal. Sorry to be ambiguous.

NicolasHug · 2019-03-24T23:57:06Z

@praths007 , as discussed here #13324 (comment), this PR would need an update to implement the following point:

raise DataConversionWarning in pairwise_distances, in case metric in PAIRWISE_BOOLEAN_FUNCTIONS and the dtype of the array isn't already bool:

The input data need to be converted to bool for boolean metric {metric}. This will create a copy. To remove this warning, explicitly convert the data using e.g. X = X.astype(np.bool) (same for Y if needed).

LMK if you need help or if you'd rather someone else take it over

merge fork

praths007 · 2019-03-27T13:57:49Z

Thank You @NicolasHug, Majority of the tests cases pass now except documentation (which I am assuming fails because the docstring has now changed from False to None). Is there someplace this change needs to be reflected .

sklearn/metrics/pairwise.py

NicolasHug · 2019-03-27T14:05:58Z

sklearn/metrics/tests/test_pairwise.py

    assert_raises(ValueError, pairwise_distances, X, Y, metric="blah")

+    # Test boolean metric raises dataconversion warning
+    assert_warns_message(DataConversionWarning, 'boolean', pairwise_distances,


please use pytest here, and a longer match for the message

sklearn/preprocessing/data.py

sklearn/utils/tests/test_validation.py

NicolasHug · 2019-03-27T14:14:19Z

sklearn/utils/tests/test_validation.py

    assert_warns(DataConversionWarning, check_array, df,
                 dtype='numeric', warn_on_dtype=True)
-    assert_no_warnings(check_array, df, dtype='object', warn_on_dtype=True)
+    with pytest.warns(DeprecationWarning,


use pytest.warn(None) and put this in a separate test if you need to ignore the deprecation warning

sklearn/utils/tests/test_validation.py

sklearn/utils/validation.py

circle ci failure fix

…to remove_warn_on_dtype_from_check_array

sklearn/metrics/tests/test_pairwise.py

sklearn/utils/tests/test_validation.py

NicolasHug

Some more comments but this is looking good

Please also add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself with :user:.

NicolasHug · 2019-03-29T10:59:02Z

sklearn/utils/validation.py

+    if warn_on_dtype is not None:
+        warnings.warn(
+            "'warn_on_dtype' is deprecated in "
+            "version 0.21 and will be removed in 0.23 "


Suggested change

"version 0.21 and will be removed in 0.23 "

"version 0.21 and will be removed in 0.23. "

sklearn/utils/validation.py

sklearn/utils/tests/test_validation.py

doc/whats_new/v0.21.rst

NicolasHug

last comments but this LGTM otherwise

Thanks @praths007 for sticking to it!!

doc/whats_new/v0.21.rst

sklearn/utils/validation.py

NicolasHug · 2019-03-29T15:11:48Z

Also please update this PR's header since it's a bit outdated now ;)

deprecated warn_on_dtype from check_array and check_X_y
the only occurrence of a DataConversionWarning is now handled directly in check_pairwise_array since this is only related to boolean metrics.

ogrisel

A bunch of mostly cosmetic comments to address before merging.

ogrisel · 2019-04-19T08:17:42Z

sklearn/metrics/pairwise.py

+
+        if dtype == bool and (X.dtype != bool or Y.dtype != bool):
+            msg = ("Data was converted to boolean "
+                   "for metric %s" % (metric))


Cosmetics: this fits on one line and there is no need to wrap the metric variable with parenthesis:

msg = "Data was converted to boolean for metric %s" % metric

ogrisel · 2019-04-19T08:19:57Z

sklearn/metrics/tests/test_pairwise.py

+    # non-boolean arrays are converted to boolean for boolean
+    # distance metrics with a data conversion warning
+    msg = ("Data was converted to boolean "
+           "for metric %s" % (metric))


Same remark:

msg = "Data was converted to boolean for metric %s" % metric

ogrisel · 2019-04-19T08:20:48Z

sklearn/metrics/tests/test_pairwise.py

+
+def test_no_data_conversion_warning():
+    # No warnings issued if metric is not a
+    # boolean distance function


Again, no need to split such comments on multiple lines before the 79 chars limit of PEP8.

ogrisel · 2019-04-19T08:28:06Z

sklearn/utils/tests/test_validation.py

    assert_raises(ValueError, assert_all_finite, X)


+@pytest.mark.filterwarnings("ignore: 'warn_on_dtype' is deprecated")  # 0.23


Instead of ignoring the warning, we should update the test to stop passing the warn_on_dtype argument as test_check_array_series is actually not concerned with the warn_on_dtype thing.

ogrisel · 2019-04-19T08:35:00Z

sklearn/utils/validation.py

+        warnings.warn(
+            "'warn_on_dtype' is deprecated in "
+            "version 0.21 and will be removed in 0.23. "
+            "Use 'warn_on_dtype=None' to remove this warning.",


This phrasing is problematic: we cannot tell people to explicitly set a parameter that will be removed at a release. This will break their code when they upgrade.

Instead we should tell them to not set the warn_on_dtype parameter.

Also please do not use that many line breaks to format this string either.

ogrisel · 2019-04-19T08:37:05Z

sklearn/utils/validation.py


+        .. deprecated:: 0.21
+            ``warn_on_dtype`` is deprecated in
+            version 0.21 and will be removed in 0.23.


PEP8 says the limit is at 79 so there is no need to break that early:

.. deprecated:: 0.21 ``warn_on_dtype`` is deprecated in version 0.21 and will be removed in 0.23.

ogrisel · 2019-04-19T08:38:20Z

sklearn/utils/validation.py


+        .. deprecated:: 0.21
+            ``warn_on_dtype`` is deprecated in
+            version 0.21 and will be removed in 0.23.


same comment here.

merge fork

merge sklearn-master

NicolasHug · 2019-04-19T18:04:12Z

Comments have been addressed, merging

Thanks @praths007 !

This reverts commit ac23178.

praths007 added 2 commits March 4, 2019 07:36

Merge pull request #1 from scikit-learn/master

ba5fd14

merge fork

added datatype conversion for arrays whose boolean distance is to be …

d9926ba

…calculated

praths007 changed the title ~~[WIP] Remove "warn_on_dtype" from check_array~~ Remove "warn_on_dtype" from check_array Mar 4, 2019

praths007 added 5 commits March 4, 2019 08:26

flake 8 changes

760a88b

Merge pull request #2 from scikit-learn/master

972d9e6

merge fork

Merge branches 'master' and 'remove_warn_on_dtype_from_check_array' of …

9d0c75d

…https://github.com/praths007/scikit-learn into remove_warn_on_dtype_from_check_array

removed warn_on_dtype from check_pairwise_array

9a73b7c

removed flake 8 issue

f2fc999

jnothman reviewed Mar 17, 2019

View reviewed changes

sklearn/metrics/pairwise.py Outdated Show resolved Hide resolved

praths007 added 4 commits March 18, 2019 04:44

restored extraneous flake 8 changes

e9a6fec

deprecated warn_on_dtype acceptance in check_array

6d45e73

flake 8 grief fix

4dcfc93

Merge pull request #3 from scikit-learn/master

80074cb

merge fork

NicolasHug reviewed Mar 18, 2019

View reviewed changes

sklearn/metrics/pairwise.py Show resolved Hide resolved

NicolasHug mentioned this pull request Mar 18, 2019

fixed issue #13324 removed warn_on_dtype references in pairwise.py #13434

Closed

praths007 added 2 commits March 19, 2019 04:46

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

0a615eb

…to remove_warn_on_dtype_from_check_array

removed uneccessary changes

db005d3

NicolasHug reviewed Mar 22, 2019

View reviewed changes

NicolasHug mentioned this pull request Mar 23, 2019

Remove "warn_on_dtype" from check_array #13324

Closed

praths007 added 2 commits March 25, 2019 06:21

incorporated review changes #1

187a2c9

Merge pull request #4 from scikit-learn/master

9120535

merge fork

NicolasHug reviewed Mar 27, 2019

View reviewed changes

praths007 added 6 commits March 28, 2019 06:25

review changes

97440aa

fixing doc

5eb8d17

fixing doc#2

660476e

removed unwanted examples

ad611e2

Merge pull request #5 from scikit-learn/master

09627dc

circle ci failure fix

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

1567f89

…to remove_warn_on_dtype_from_check_array

NicolasHug reviewed Mar 28, 2019

View reviewed changes

sklearn/metrics/tests/test_pairwise.py Outdated Show resolved Hide resolved

sklearn/metrics/tests/test_pairwise.py Outdated Show resolved Hide resolved

sklearn/utils/tests/test_validation.py Outdated Show resolved Hide resolved

review changes

350ef5c

NicolasHug reviewed Mar 29, 2019

View reviewed changes

praths007 added 2 commits March 29, 2019 08:22

restored test, updated changelog, modified docstring

69981aa

added .

62d2834

NicolasHug reviewed Mar 29, 2019

View reviewed changes

doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved

updated changelog

1a39743

NicolasHug approved these changes Mar 29, 2019

View reviewed changes

doc/whats_new/v0.21.rst Show resolved Hide resolved

sklearn/utils/validation.py Outdated Show resolved Hide resolved

sklearn/utils/validation.py Outdated Show resolved Hide resolved

sklearn/utils/validation.py Outdated Show resolved Hide resolved

updated changelog

4943d1e

NicolasHug changed the title ~~Remove "warn_on_dtype" from check_array~~ [MRG + 1] Remove "warn_on_dtype" from check_array Apr 5, 2019

ogrisel approved these changes Apr 19, 2019

View reviewed changes

praths007 added 3 commits April 19, 2019 07:18

cosmetic changes

6e0b773

Merge pull request #6 from scikit-learn/master

9f26b49

merge fork

Merge pull request #7 from praths007/master

57b1f04

merge sklearn-master

NicolasHug merged commit 4b9e12e into scikit-learn:master Apr 19, 2019

jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Apr 25, 2019

Deprecate "warn_on_dtype" from check_array (scikit-learn#13382)

62b5a85

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Deprecate "warn_on_dtype" from check_array (scikit-learn#13382)

ac23178

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "Deprecate "warn_on_dtype" from check_array (scikit-learn#13382)"

4a110c4

This reverts commit ac23178.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "Deprecate "warn_on_dtype" from check_array (scikit-learn#13382)"

08241de

This reverts commit ac23178.

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

Deprecate "warn_on_dtype" from check_array (scikit-learn#13382)

2449153

	"version 0.21 and will be removed in 0.23 "
	"version 0.21 and will be removed in 0.23. "

		assert_raises(ValueError, assert_all_finite, X)


		@pytest.mark.filterwarnings("ignore: 'warn_on_dtype' is deprecated") # 0.23

Uh oh!

Conversation

praths007 commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs:

Implemented changes:

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NicolasHug commented Mar 18, 2019

Uh oh!

jnothman commented Mar 19, 2019 via email

Uh oh!

praths007 commented Mar 19, 2019

Uh oh!

NicolasHug commented Mar 19, 2019

Uh oh!

jnothman commented Mar 19, 2019 via email

Uh oh!

praths007 commented Mar 22, 2019

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman commented Mar 23, 2019 via email

Uh oh!

NicolasHug commented Mar 24, 2019

Uh oh!

praths007 commented Mar 27, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasHug commented Mar 29, 2019

praths007 commented Mar 4, 2019 •

edited

Loading

NicolasHug commented Apr 19, 2019 •

edited

Loading