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

Respect copy=False in astype #328

Merged
merged 2 commits into from
Mar 3, 2020
Merged

Conversation

eric-wieser
Copy link
Contributor

Previously this would make a copy anyway.

Previously this would make a copy anyway.
@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

Merging #328 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
- Coverage   93.98%   93.94%   -0.04%     
==========================================
  Files          19       19              
  Lines        2327     2329       +2     
==========================================
+ Hits         2187     2188       +1     
- Misses        140      141       +1

@@ -2186,6 +2186,9 @@ def astype(self, dtype, copy=True):
:obj:`COO.elemwise`: Apply an arbitrary element-wise function to one or two
arguments.
"""
# this matches numpy's behavior
if self.dtype == dtype and copy == False:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing dtypes can be tricky. To be safe:

if not copy and self.dtype.type == np.dtype(dtype).type:
    return self

Copy link
Contributor Author

@eric-wieser eric-wieser Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion doesn't match the behavior of regular numpy arrays.

Worse, it does the wrong thing for reversed-endian types.

The one thing your approach does ensure is that intc, int_, and longlong are respected by identity - but numpy doesn't do that all over the place.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll amend my recommendation to only coerce the user input, then:

if not copy and self.dtype == np.dtype(dtype):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That coercion happens automatically anyway, and is probably faster at the C level where it doesn't need a python function call.

@hameerabbasi hameerabbasi merged commit d1fa248 into pydata:master Mar 3, 2020
@eric-wieser eric-wieser deleted the patch-2 branch March 3, 2020 15:55
@eric-wieser
Copy link
Contributor Author

Thanks!

@hameerabbasi
Copy link
Collaborator

Thank you, @eric-wieser!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants