API: Sparse Return Types

[gfyoung]

The documentation of SparseArray.cumsum says it returns a Series, but that is not the case:

>>> from pandas import SparseArray, np
>>> data = np.arange(10)
>>> s = SparseArray(data, fill_value=np.nan)
>>> type(s.cumsum())
<class 'pandas.sparse.array.SparseArray'>
>>>
>>> s = SparseArray(data, fill_value=2)
>>> type(s.cumsum())
<class 'numpy.ndarray'>

Given that to_dense now returns self.values and ignores fill_value, this current behaviour in cumsum seems illogical and should probably just return SparseArray regardless. At the very least though, I think a documentation fix is in order.

EDIT: there is a similar issue with SparseSeries that should also be addressed

[jreback]

these were all JUST fixed by @sinhrks if you would like to make a master documentation issue, pls do. but leave it as this one.

[gfyoung]

@jreback : Yes, I am aware of what @sinhrks did, as this is against the master branch. He did not make any changes to cumsum in his PR.

[jreback]

ok, then if you find more things. just add them at the top, DONT't create more issues about this

[jreback]

though this is actually not a documentation issue at all. but a functional one.

[gfyoung]

@jreback : well at the very least, the documentation should be changed to match?

[jreback]

@gfyoung ABOSOLUTEY NOT. Its an issue, you don't just change the documentation you fix the problem!

[gfyoung]

I'm confused. We're currently providing incorrect information in the documentation...I meant this should be a temporary measure.

[jreback]

@gfyoung how are you confused? its an api issue, changing the documentation (which is only inherited) is pointless. you can certainly propose a fix for the issue. fixing documentation is a band-aid which is not even worth doing.

[gfyoung]

@jreback : If you had just said, "it's not worthwhile to change the documentation as a stopgap measure even though it is incorrect," instead of your last two comments, that would have cleared up my confusion.

[sinhrks]

@gfyoung thanks for the catch. Can it be included in #12810? Lmk if any fix is needed on my side.

[gfyoung]

@sinhrks : #12810 (diff)

[gfyoung]

Looking at this issue again, I must say I find this SparseArray.cumsum API quite confusing. Isn't the point of this function to provide a cumulative sum as we slowly progress through the array using the sum function defined for SparseArray i.e.:

>>> s = SparseArray([1, 2, 2, np.nan, 2, 2, np.nan], fill_value=2)
>>> s.sum()
9.0
>>> s.cumsum()
array([1., 3., 5., nan, nan, nan, nan])

I would have expected the following:

array([1., 3., 5., nan, nan, 7.0, 9.0, nan])

EDIT: Same is true for SparseSeries

@jreback , @jorisvandenbossche , @sinhrks : Thoughts?

[sinhrks]

@gfyoung +1. We should make an agreement that SparseArray is compat with pandas internals rather than numpy.

pd.Series([1, 2, 2, np.nan, 2, 2, np.nan]).cumsum()
# 0    1.0
# 1    3.0
# 2    5.0
# 3    NaN
# 4    7.0
# 5    9.0
# 6    NaN
# dtype: float64

np.cumsum([1, 2, 2, np.nan, 2, 2, np.nan])
# array([  1.,   3.,   5.,  nan,  nan,  nan,  nan])

[gfyoung]

@sinhrks : I was about to implement a fix, but what is the meaning of fill_value exactly? Because I thought it should have an impact on aggregation behaviour:

>>> data = np.arange(10)
>>> s = SparseArray(data, fill_value=2)
>>> s.sum()
45
>>> s = SparseArray(data, fill_value=np.nan)
>>> s.sum()
45

IIUC, fill_value=2 implies that any data point with "2" as the value is considered a null / missing data point, meaning that the first sum should be 43?

[sinhrks]

fill_value is unrelated to normal missing values. It is to specify values omitted in sparse repr. This shouldn't affect to dense / aggregated results.

# this omits np.nan, holds [1, 1] as sparse values with its locations
pd.SparseArray([np.nan, np.nan, 1, 1], fill_value=np.nan)
# [nan, nan, 1.0, 1.0]
# Fill: nan
# IntIndex
# Indices: array([2, 3])

# this omits 1, holds [nan, nan] as sparse values with its locations
pd.SparseArray([np.nan, np.nan, 1, 1], fill_value=1)
# [nan, nan, 1, 1]
# Fill: 1
# IntIndex
# Indices: array([0, 1])

[gfyoung]

@sinhrks : Okay, so the sums should be the same?

[sinhrks]

Yes, should be the same.