[MRG+1] Use astype(.., copy=False) when possible #11973

rth · 2018-09-02T13:52:25Z

This uses X.astype(..., copy=False) when possible to avoid memory copies.

Closes #11970

I have manually reviewed all cases of .astype and manually added copy=False when this makes sense (i.e. when the array can be of the same dtype as the target dtype)

To support the copy argument for both sparse and dense arrays, a new utils.fixes._astype_copy_false function is added. It can be used as,

X = X.astype('float32', **_astype_copy_false(X))

This is somewhat awkward, however it makes it explicit that this is a temporary fix that can be dropped once we only support scipy > 1.1 and IMO is better in this sense than,

if X.dtype != np.float32:
   X = X.astype('float32', copy=False)

as the latter essentially permanently re-implements some checks from numpy/scipy.

rth · 2018-09-02T15:22:26Z

Circle CI python 2 build failed due to https://github.com/tomMoral/loky/issues/151 as it did in #11899 (comment) in the Circle CI running on my fork. Interestingly it didn't fail there when merged into master.

glemaitre · 2018-09-13T16:05:29Z

sklearn/utils/fixes.py

+
+# TODO: replace by copy=False, when only scipy > 1.1 is supported.
+def _astype_copy_false(X):
+    """Returns the copy=False parameter for {ndarray,csr_matrix,csc_matrix}.astype


Thanks for the review! You are right, it's too long (although technically it passed flake8) -- Fixed.

glemaitre · 2018-09-13T16:15:55Z

sklearn/impute.py

@@ -338,7 +338,7 @@ def _dense_fit(self, X, strategy, missing_values, fill_value):
                most_frequent = np.empty(X.shape[0])

            for i, (row, row_mask) in enumerate(zip(X[:], mask[:])):
-                row_mask = np.logical_not(row_mask).astype(np.bool)
+                row_mask = np.logical_not(row_mask).astype(np.bool, copy=False)


The code review capability of your editor must be adding extra spaces :)

This is 79 character long and valid according to PEP8.

Ups got trapped by my own editor :)

Actually we don't need astype here since row_mask will always be bool since mask is bool. However, there is a astype in fixes which might changed:

scikit-learn/sklearn/utils/fixes.py

Line 321 in 73a5592

return np.frompyfunc(lambda x: x != x, 1, 1)(X).astype(bool)

glemaitre · 2018-09-13T21:12:40Z

sklearn/preprocessing/imputation.py

@@ -289,7 +289,7 @@ def _dense_fit(self, X, strategy, missing_values, axis):
            most_frequent = np.empty(X.shape[0])

            for i, (row, row_mask) in enumerate(zip(X[:], mask[:])):
-                row_mask = np.logical_not(row_mask).astype(np.bool)
+                row_mask = np.logical_not(row_mask).astype(np.bool, copy=False)


Do we change a deprecate file?

glemaitre · 2018-09-13T21:22:53Z

sklearn/cluster/hierarchical.py

@@ -487,7 +489,8 @@ def linkage_tree(X, connectivity=None, n_components='deprecated',
    del diag_mask

    if affinity == 'precomputed':
-        distances = X[connectivity.row, connectivity.col].astype('float64')
+        distances = X[connectivity.row, connectivity.col].astype(
+                'float64', **_astype_copy_false(X))


flake8 tell me 4 spaces less ;)

glemaitre · 2018-09-13T21:49:17Z

LGTM. if you could have a pass on the following occurences. I am not 100% sure that they should change.

test_hierarchical.py -> l. 302
test_k_means.py -> l. 806
test_svmlight_format.py -> l. 231 - 261 - 263 - 267 - 269
test_spectral_embedding.py -> l. 125
supervised.py -> l. 856
metrics/tests/test_classification -> l. 724 - 727

jnothman

It is hard to review this very thoroughly, so I would be unsure if there are cases where a copy was intended. I would be happy to merge it into master (but not 0.20.X) and see if any users complain of breakage in 0.21dev.

glemaitre · 2018-09-15T18:21:59Z

I think that @jnothman is right. Actually, we could merge it now without backport in 0.20.X.

jnothman · 2018-09-16T01:14:42Z

Do you want to add a what's new?

rth · 2018-09-26T21:07:11Z

Thanks for the review!

Addressed some of @glemaitre 's comments, rebased (to include the v0.21 what's new template) and added a what's new.

jnothman · 2018-09-26T23:58:49Z

As I implied, I can't claim to thoroughly review this with confidence, but I'm okay to merge it on trust and the test of time, once we've dropped Py2 support.

jnothman · 2019-01-06T10:11:27Z

CI failing, @rth

rth · 2019-02-27T17:54:26Z

Rebased and fixed CI.

CI is in progress but it should pass (last commit only linting failed).

GaelVaroquaux · 2019-02-28T14:10:02Z

sklearn/cluster/hierarchical.py

@@ -109,7 +111,7 @@ def _single_linkage_tree(connectivity, n_samples, n_nodes, n_clusters,

    # Convert edge list into standard hierarchical clustering format
    single_linkage_tree = _hierarchical._single_linkage_label(mst_array)
-    children_ = single_linkage_tree[:, :2].astype(np.int)
+    children_ = single_linkage_tree[:, :2].astype(np.int, copy=False)


Actually, here I think that it's beneficial to have a copy. It's a somewhat subtle reason:

We are taking a view of the data that is smaller than the original data, via the slicing. This view references the original data. If no copy is made, the original data cannot be garbage collected, and memory is wasted.

to fix or not?

Will fix tomorrow...

Agreed, removed the copy=False here.

GaelVaroquaux · 2019-02-28T14:12:58Z

sklearn/cluster/hierarchical.py

@@ -229,7 +231,7 @@ def ward_tree(X, connectivity=None, n_clusters=None, return_distance=False):
                          stacklevel=2)
        X = np.require(X, requirements="W")
        out = hierarchy.ward(X)
-        children_ = out[:, :2].astype(np.intp)
+        children_ = out[:, :2].astype(np.intp, copy=False)


Whether or not the argument about view not being released applies here or not depends on whether we have return_distance or not. I am not sure whether or not the copy should be kept or not.

This comment does not ask for a change in the diff. It's just a reflection as I pass by.

Removed copy=False here to use the defaults when in doubt.

GaelVaroquaux

LGTM.

I did make a comment that I think would be an improvement. But if y'all think that it's a nitpick, I don't mind merging without taking in account my comment.

GaelVaroquaux · 2019-03-01T10:07:05Z

Awesome. Merging!

* Use astype(.., copy=False) when possible * Use Use astype(.., copy=False) also in tests * Fix CI * Guillaume's comments * Address review comments * Add what's new * Fix CI * Lint * Fix merge issues * More merge conflict fixes * Fix failing test * Lint * Use copy=True in cluster/hierarchical * More fixes

…n#11973)" This reverts commit 0ed6b19.

* Use astype(.., copy=False) when possible * Use Use astype(.., copy=False) also in tests * Fix CI * Guillaume's comments * Address review comments * Add what's new * Fix CI * Lint * Fix merge issues * More merge conflict fixes * Fix failing test * Lint * Use copy=True in cluster/hierarchical * More fixes

rth changed the title ~~Use astype(.., copy=False) when possible~~ [MRG] Use astype(.., copy=False) when possible Sep 2, 2018

rth requested a review from glemaitre September 13, 2018 15:57

glemaitre reviewed Sep 13, 2018

View reviewed changes

jnothman reviewed Sep 15, 2018

View reviewed changes

rth force-pushed the use-asarray-copy-false branch from 1fd1ed5 to faa16a1 Compare September 26, 2018 21:02

jnothman approved these changes Oct 7, 2018

View reviewed changes

rth force-pushed the use-asarray-copy-false branch from faa16a1 to 40f4e35 Compare October 14, 2018 13:16

rth and others added 8 commits February 27, 2019 15:32

Use astype(.., copy=False) when possible

35a0c2e

Use Use astype(.., copy=False) also in tests

3e13eb9

Fix CI

5eee049

Guillaume's comments

eebac2c

Address review comments

5b7b7cc

Add what's new

85d17bc

Fix CI

da4b302

Lint

3d2cf40

rth force-pushed the use-asarray-copy-false branch from b2db080 to 3d2cf40 Compare February 27, 2019 15:20

rth added 4 commits February 27, 2019 16:24

Fix merge issues

0787a5a

More merge conflict fixes

4646d2f

Fix failing test

0777396

Lint

17ae637

agramfort approved these changes Feb 28, 2019

View reviewed changes

agramfort changed the title ~~[MRG] Use astype(.., copy=False) when possible~~ [MRG+1] Use astype(.., copy=False) when possible Feb 28, 2019

GaelVaroquaux reviewed Feb 28, 2019

View reviewed changes

GaelVaroquaux approved these changes Feb 28, 2019

View reviewed changes

rth added 2 commits March 1, 2019 09:04

Use copy=True in cluster/hierarchical

fb2f005

More fixes

b4ed165

GaelVaroquaux merged commit cf9a740 into scikit-learn:master Mar 1, 2019

rth deleted the use-asarray-copy-false branch March 1, 2019 10:08

OGordon100 mentioned this pull request Mar 5, 2019

ENH: Allow inplace copying in place in "detrend" function scipy/scipy#9792

Merged

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

Revert "[MRG+1] Use astype(.., copy=False) when possible (scikit-lear…

26b0f2a

…n#11973)" This reverts commit 0ed6b19.

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

Revert "[MRG+1] Use astype(.., copy=False) when possible (scikit-lear…

2e15481

…n#11973)" This reverts commit 0ed6b19.

Uh oh!

[MRG+1] Use astype(.., copy=False) when possible #11973

[MRG+1] Use astype(.., copy=False) when possible #11973

Uh oh!

Conversation

rth commented Sep 2, 2018

Uh oh!

rth commented Sep 2, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Sep 13, 2018

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Sep 15, 2018

Uh oh!

jnothman commented Sep 16, 2018

Uh oh!

rth commented Sep 26, 2018

Uh oh!

jnothman commented Sep 26, 2018

Uh oh!

jnothman commented Jan 6, 2019

Uh oh!

rth commented Feb 27, 2019

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!

GaelVaroquaux left a comment

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Mar 1, 2019

Uh oh!

Uh oh!