-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
API: Return sparse objects always for cumsum #14771
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 all commits
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 |
---|---|---|
|
@@ -630,21 +630,29 @@ def take(self, indices, axis=0, convert=True, *args, **kwargs): | |
|
||
def cumsum(self, axis=0, *args, **kwargs): | ||
""" | ||
Cumulative sum of values. Preserves locations of NaN values | ||
Cumulative sum of non-NA/null values. | ||
|
||
When performing the cumulative summation, any non-NA/null values will | ||
be skipped. The resulting SparseSeries will preserve the locations of | ||
NaN values, but the fill value will be `np.nan` regardless. | ||
|
||
Parameters | ||
---------- | ||
axis : {0} | ||
|
||
Returns | ||
------- | ||
cumsum : SparseSeries if `self` has a null `fill_value` and a | ||
generic Series otherwise | ||
cumsum : SparseSeries | ||
""" | ||
nv.validate_cumsum(args, kwargs) | ||
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. e.g. I would do and something similar to We normally don't list these in the doc-string Parameters (even though they are in the signature) 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. Fair enough, though it was misleading because I had initially assumed the |
||
new_array = SparseArray.cumsum(self.values) | ||
if isinstance(new_array, SparseArray): | ||
return self._constructor( | ||
new_array, index=self.index, | ||
sparse_index=new_array.sp_index).__finalize__(self) | ||
# TODO: gh-12855 - return a SparseSeries here | ||
return Series(new_array, index=self.index).__finalize__(self) | ||
if axis is not None: | ||
axis = self._get_axis_number(axis) | ||
|
||
new_array = self.values.cumsum() | ||
|
||
return self._constructor( | ||
new_array, index=self.index, | ||
sparse_index=new_array.sp_index).__finalize__(self) | ||
|
||
@Appender(generic._shared_docs['isnull']) | ||
def isnull(self): | ||
|
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.
side thing. we should use
_constructor
(and define it forSparseArray
).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.
can make a separate issue / do later
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.
Sounds good as a follow-up.