[MRG + 1] Remove "warn_on_dtype" from check_array #13382

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

@@ -417,6 +418,11 @@ def test_check_array_dtype_warning():
    assert_equal(X_checked.format, 'csr')


+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

@@ -122,6 +122,10 @@ def test_pairwise_distances():
    # Test that a value error is raised if the metric is unknown
    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

@@ -712,7 +725,10 @@ def test_check_dataframe_warns_on_dtype():
                         check_array, df, dtype=np.float64, warn_on_dtype=True)
    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

@@ -687,6 +701,7 @@ def test_suppress_validation():
    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

        Raise DataConversionWarning if the dtype of the input data structure
        does not match the requested dtype, causing a memory copy.

+        .. 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

        Raise DataConversionWarning if the dtype of the input data structure
        does not match the requested dtype, causing a memory copy.

+        .. 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 Outdated 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

A867 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. "

		@@ -687,6 +701,7 @@ def test_suppress_validation():
		assert_raises(ValueError, assert_all_finite, X)


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

Uh oh!

[MRG + 1] Remove "warn_on_dtype" from check_array #13382

[MRG + 1] Remove "warn_on_dtype" from check_array #13382

Uh oh!

Conversation

Uh oh!

Reference Issues/PRs:

Implemented changes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!