Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from Mar 1, 2019

Conversation

5 participants
@rth
Copy link
Member

rth commented Sep 2, 2018

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 rth changed the title Use astype(.., copy=False) when possible [MRG] Use astype(.., copy=False) when possible Sep 2, 2018

@rth

This comment has been minimized.

Copy link
Member Author

rth commented Sep 2, 2018

Circle CI python 2 build failed due to tomMoral/loky#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.

@rth rth requested a review from glemaitre Sep 13, 2018


# 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

This comment has been minimized.

@glemaitre

glemaitre Sep 13, 2018

Contributor

PEP8

This comment has been minimized.

@rth

rth Sep 13, 2018

Author Member

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

@@ -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)

This comment has been minimized.

@glemaitre

glemaitre Sep 13, 2018

Contributor

PEP8

This comment has been minimized.

@rth

rth Sep 13, 2018

Author Member

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

This is 79 character long and valid according to PEP8.

This comment has been minimized.

@glemaitre

glemaitre Sep 13, 2018

Contributor

Ups got trapped by my own editor :)

This comment has been minimized.

@glemaitre

glemaitre Sep 13, 2018

Contributor

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:

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

@@ -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)

This comment has been minimized.

@glemaitre

glemaitre Sep 13, 2018

Contributor

Do we change a deprecate file?

@@ -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))

This comment has been minimized.

@glemaitre

glemaitre Sep 13, 2018

Contributor

flake8 tell me 4 spaces less ;)

@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Sep 13, 2018

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
Copy link
Member

jnothman left a comment

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

This comment has been minimized.

Copy link
Contributor

glemaitre commented Sep 15, 2018

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

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Sep 16, 2018

Do you want to add a what's new?

@rth rth force-pushed the rth:use-asarray-copy-false branch from 1fd1ed5 to faa16a1 Sep 26, 2018

@rth

This comment has been minimized.

Copy link
Member Author

rth commented Sep 26, 2018

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

This comment has been minimized.

Copy link
Member

jnothman commented Sep 26, 2018

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.

@rth rth force-pushed the rth:use-asarray-copy-false branch from faa16a1 to 40f4e35 Oct 14, 2018

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Jan 6, 2019

CI failing, @rth

@rth rth force-pushed the rth:use-asarray-copy-false branch from b2db080 to 3d2cf40 Feb 27, 2019

rth added some commits Feb 27, 2019

@rth

This comment has been minimized.

Copy link
Member Author

rth commented Feb 27, 2019

Rebased and fixed CI.

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

@rth rth added this to To do in Sprint Paris 2019 via automation Feb 27, 2019

@rth rth moved this from To do to Needs review in Sprint Paris 2019 Feb 27, 2019

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

@@ -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)

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Feb 28, 2019

Member

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.

This comment has been minimized.

@agramfort

agramfort Feb 28, 2019

Member

to fix or not?

This comment has been minimized.

@rth

rth Feb 28, 2019

Author Member

Will fix tomorrow...

This comment has been minimized.

@rth

rth Mar 1, 2019

Author Member

Agreed, removed the copy=False here.

@@ -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)

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Feb 28, 2019

Member

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.

This comment has been minimized.

@rth

rth Mar 1, 2019

Author Member

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

@GaelVaroquaux
Copy link
Member

GaelVaroquaux left a comment

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.

rth added some commits Mar 1, 2019

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Mar 1, 2019

Awesome. Merging!

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

9 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 92.75% of diff hit (target 92.55%)
Details
codecov/project Absolute coverage decreased by -0.33% but relative coverage increased by +0.19% compared to afc6cc5
Details

Sprint Paris 2019 automation moved this from Needs review to Done Mar 1, 2019

@rth rth deleted the rth:use-asarray-copy-false branch Mar 1, 2019

Kiku-git added a commit to Kiku-git/scikit-learn that referenced this pull request Mar 4, 2019

[MRG+1] Use astype(.., copy=False) when possible (scikit-learn#11973)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.