REGR: use dtype.fill_value in ExtensionBlock.fill_value where available

[scottgigante]

• closes Filtering dataframe with sparse column leads to NAs in sparse column #27781, closes Sparse DataFrame with all-zeros column returns NA for fancy index #29563
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[scottgigante]

I don't have authority to request reviews, so @TomAugspurger @mroeschke @jorisvandenbossche

[jorisvandenbossche]

I am not sure this fix is entirely correct. While it certainly fixed the original buggy case, like:

In [7]: df1 = pd.DataFrame({"A": pd.SparseArray([0, 0, 0]), 'B': [1,2,3]}) 

In [8]: df1.loc[df1['B'] != 2]       
Out[8]: 
   A  B
0  0  1
2  0  3

(before, this gave NaNs in the first column).

But it also seems to introduce a new bug, eg when doing a reindex:

In [9]: df1.reindex([0,1,3]) 
Out[9]: 
   A    B
0  0  1.0
1  0  2.0
3  0  NaN

This should give a NaN and not 0 at row 3, I think?

[scottgigante]

So here's the problem as far as I understand: in https://github.com/pandas-dev/pandas/blob/master/pandas/core/internals/blocks.py#L1219 , blk.fill_value is used to mean the value which is used when you take an index that doesn't exist. In sparse, dtype.fill_value is used to mean the value which is used for filling sparse entries. I don't see an easy way of disentangling these.

[jorisvandenbossche]

The naming is potentially a bit confusing here (fill_value in take is generally pointing to what is called "na_value" elsewhere).
But so the question is: for SparseArray, should we be using the "SparseDtype.fill_value" when doing a reindex, or rather the "SparseDtype.na_value". It's not directly clear to me which of the two is correct / desired.

Using "na_value" feels more correct (that's what reindexing does for all other array types), but using "fill_value" can avoid densifying the sparse array in a reindexing operation.

Looking at SparseArray.take itself:

In [15]: a = pd.arrays.SparseArray([1.0, 0.0, np.nan, 0.0], fill_value=0.0)    

In [16]: a       
Out[16]: 
[1.0, 0.0, nan, 0.0]
Fill: 0.0
IntIndex
Indices: array([0, 2], dtype=int32)

In [18]: a.take([0, 1, -1, -1], allow_fill=True)                                                                                                                                                                   
Out[18]: 
[1.0, 0.0, nan, nan]
Fill: 0.0
IntIndex
Indices: array([0, 2, 3], dtype=int32)

Here it is clearly "na_value" that is used by default.

[scottgigante]

I'm fundamentally unsure how to make this happen, if this is the desired functionality:

>>> df1 = pd.DataFrame({"A": pd.SparseArray([0, 0, 0]), 'B': [1,2,3]}) 
>>> df1.reindex([0,1,3]) 
   A    B
0  0  1.0
1  0  2.0
3  NaN  NaN

[jreback]

this is fine, can merge on green.

[jreback]

thanks @scottgigante

[jorisvandenbossche]

@jreback as shown by the discussion above (and @scottgigante's "I'm fundamentally unsure how to make this happen"), this PR was clearly not ready to be merged.

@scottgigante sorry for the slow follow-up. It's however a complicated topic with not directly a clear easy solution (as you experienced yourself), so it requires some time to delve in, which I didn't find yet

[jorisvandenbossche]

@jreback can you react here? Otherwise I am planning to revert this change. It's not clear to me that this is the right fix, as it possibly causes another regression

[jreback]

@jorisvandenbossche I am unclear what you think is the problem. This closed a few issues w/o causing others. If you want to revert ok by me, but the soln looked fine.

[scottgigante]

@jreback @jorisvandenbossche it's not clear to me that the (unintended) consequence of this PR is a regression but it's worth discussing: what should be the outcome of this action?

df1 = pd.DataFrame({"A": pd.arrays.SparseArray([1, 2, 0]), 'B': [1,2,3]})
df1.reindex([0,1,3])

Current behaviour:

>>> df1.reindex([0,1,3])
    A    B
0 1.0  1.0
1 2.0  2.0
3 NaN  NaN
>>> df1.reindex([0, 1, 3])['A'].sparse.density
1.0

New behaviour:

>>> df1.reindex([0,1,3])
    A    B
0 1.0  1.0
1 2.0  2.0
3 0.0  NaN
>>> df1.reindex([0, 1, 3])['A'].sparse.density
0.6666666666666666

I don't know that filling a sparse array with non-sparse values when reindexing by a nonexistent index is the right thing to do, but if that is desirable then this is a regression (and I don't know how to fix it).

[jorisvandenbossche]

Sparse aside: in principle, reindexing always introduces missing values when new labels are present in the indexer.
(as the non sparse column B does in the example above)

So the question for sparse is: do we follow that rule, or do we deviate by using the "fill value" instead of a "missing value" (na_value).

And as I showed in #34158 (comment), SparseArray.take uses na_value and not fill_value. If we think this behaviour is correct, then reindex should also do that IMO

[TomAugspurger]

Reindex should always introduce missing values. This behavior on master looks incorrect:

In [6]: pd.Series([0, 1, 2], dtype=pd.SparseDtype(int)).reindex([0, 1, 3])
Out[6]:
0    0.0
1    1.0
3    NaN
dtype: Sparse[float64, 0]

In [7]: pd.DataFrame({"A": pd.SparseArray([0, 1, 2], dtype=pd.SparseDtype(int))}).reindex([0, 1, 3])
/Users/taugspurger/.virtualenvs/pandas-dev/bin/ipython:1: FutureWarning: The pandas.SparseArray class is deprecated and will be removed from pandas in a future version. Use pandas.arrays.SparseArray instead.
  #!/Users/taugspurger/Envs/pandas-dev/bin/python
Out[7]:
   A
0  0
1  1
3  0

In [8]: _.dtypes
Out[8]:
A    Sparse[int64, 0]
dtype: object

Series is correct, but DataFrame is filling with fill_value rather than the na value.