-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CoW: Switch to copy=False everywhere for Series constructor #52068
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1501,7 +1501,9 @@ def value_counts(self, dropna: bool = True) -> Series: | |
ix = coerce_indexer_dtype(ix, self.dtype.categories) | ||
ix = self._from_backing_data(ix) | ||
|
||
return Series(count, index=CategoricalIndex(ix), dtype="int64", name="count") | ||
return Series( | ||
count, index=CategoricalIndex(ix), dtype="int64", name="count", copy=False | ||
) | ||
|
||
# error: Argument 2 of "_empty" is incompatible with supertype | ||
# "NDArrayBackedExtensionArray"; supertype defines the argument type as | ||
|
@@ -1759,7 +1761,9 @@ def _values_for_rank(self): | |
# reorder the categories (so rank can use the float codes) | ||
# instead of passing an object array to rank | ||
values = np.array( | ||
self.rename_categories(Series(self.categories).rank().values) | ||
self.rename_categories( | ||
Series(self.categories, copy=False).rank().values | ||
) | ||
) | ||
return values | ||
|
||
|
@@ -2504,15 +2508,15 @@ def codes(self) -> Series: | |
""" | ||
from pandas import Series | ||
|
||
return Series(self._parent.codes, index=self._index) | ||
return Series(self._parent.codes, index=self._index, copy=False) | ||
|
||
def _delegate_method(self, name, *args, **kwargs): | ||
from pandas import Series | ||
|
||
method = getattr(self._parent, name) | ||
res = method(*args, **kwargs) | ||
if res is not None: | ||
return Series(res, index=self._index, name=self._name) | ||
return Series(res, index=self._index, name=self._name, copy=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are sure that none of the methods where this is used give a view? (didn't check in detail where this is used, just wondering) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so, but not 100%. Lets leave this out for now |
||
|
||
|
||
# utility routines | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,7 +195,7 @@ def coo_to_sparse_series( | |
from pandas import SparseDtype | ||
|
||
try: | ||
ser = Series(A.data, MultiIndex.from_arrays((A.row, A.col))) | ||
ser = Series(A.data, MultiIndex.from_arrays((A.row, A.col)), copy=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this keyword get exposed to the user (and with a default of copy=True) ? I would say this is a similar case as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The astype below creates a copy as far as I understand, so copy=False here should be good. |
||
except AttributeError as err: | ||
raise TypeError( | ||
f"Expected coo_matrix. Got {type(A).__name__} instead." | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,7 +102,9 @@ def _delegate_property_get(self, name): | |
else: | ||
index = self._parent.index | ||
# return the result as a Series, which is by definition a copy | ||
result = Series(result, index=index, name=self.name).__finalize__(self._parent) | ||
result = Series(result, index=index, name=self.name, copy=False).__finalize__( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment just above should be updated then. But also wondering if we have properties that can be a view, in which case we shouldn't necessarily do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking through the datetimelike properties/methods, I think theoretically things like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually they return views if it's a no-op. So will remove the copy=False for now |
||
self._parent | ||
) | ||
|
||
# setting this object will show a SettingWithCopyWarning/Error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sidenote: seeing this here, this might also be a case to look into if we can raise a "chained assignment" error |
||
result._is_copy = ( | ||
|
@@ -130,9 +132,9 @@ def _delegate_method(self, name, *args, **kwargs): | |
if not is_list_like(result): | ||
return result | ||
|
||
result = Series(result, index=self._parent.index, name=self.name).__finalize__( | ||
self._parent | ||
) | ||
result = Series( | ||
result, index=self._parent.index, name=self.name, copy=False | ||
).__finalize__(self._parent) | ||
|
||
# setting this object will show a SettingWithCopyWarning/Error | ||
result._is_copy = ( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should maybe not be a copy=False? (modifying the series should never mutate those codes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point