Repository: pandas-dev/pandas
Pull request: DEPR: require SparseDtype.fill_value be compatible with SparseDtype.subtype
- [x] closes #23124 (Replace xxxx with the GitHub issue number)
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [ ] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [ ] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Problem statement:
Require the dtype of SparseArray.fill_value and sp_values.dtype to match
This has confused me for a while, but we apparently (sometimes?) allow a `fill_value` whose dtype is not the same as `sp_values.dtype`

```python
In [20]: a = pd.SparseArray([1, 2, 3], fill_value=1.0)

In [21]: a
Out[21]:
[1.0, 2, 3]
Fill: 1.0
IntIndex
Indices: array([1, 2], dtype=int32)

In [22]: a.sp_values.dtype
Out[22]: dtype('int64')
```

This can lead to confusing behavior when doing operations.

I suspect a primary motivation was supporting sparse integer values with `NaN` for a fill value. We should investigate what's tested, part of the API, and useful to users.

Hints from issue discussion:
> we apparently (sometimes?) allow a fill_value whose dtype is not the same as sp_values.dtype

It was indeed allowed in some places, but in previous releases, I think it was mostly explicitly not allowed:

(examples are using 0.23.4)

```
In [5]: a = pd.SparseArray([1, 0, 0, 1, 0])

In [6]: a
Out[6]: 
[1, 0, 0, 1, 0]
Fill: 0
IntIndex
Indices: array([0, 3], dtype=int32)

In [7]: a.fill_value = np.nan
...
ValueError: unable to set fill_value nan to int64 dtype
```

```
In [8]: a = pd.SparseArray([1, np.nan, np.nan, 1, np.nan])

In [9]: a
Out[9]: 
[1.0, nan, nan, 1.0, nan]
Fill: nan
IntIndex
Indices: array([0, 3], dtype=int32)

In [10]: a.astype(int)
...
ValueError: unable to coerce current fill_value nan to int64 dtype
```

One place where we did not check it, was the constructor. However, I think it is only possible to that way create an array with an incompatible fill_value if it is fully dense?

```
In [11]: a = pd.SparseArray([1, 0, 0, 1], fill_value=np.nan)

In [12]: a
Out[12]: 
[1, 0, 0, 1]
Fill: nan
IntIndex
Indices: array([0, 1, 2, 3], dtype=int32)
```

Because if there is already a `np.nan` in the list, the dtype of the data will be float64, and then np.nan is not an incompatible fill_value.

---

On master we now allow this, but this gives rise to inconsistencies:

(examples are using master now)

```
In [2]: a = pd.SparseArray([1, 0, 0, 1])

In [3]: a.fill_value = np.nan

In [4]: a
Out[4]: 
[1, nan, nan, 1]
Fill: nan
IntIndex
Indices: array([0, 3], dtype=int32)

In [5]: a.dtype
Out[5]: Sparse[int64, nan]

In [6]: a.to_dense()
Out[6]: 
array([                   1, -9223372036854775808, -9223372036854775808,
                          1])

In [7]: np.array(a)
Out[7]: array([  1.,  nan,  nan,   1.])

In [8]: np.array(a).dtype
Out[8]: dtype('float64')
```

So you could of course say that  `to_dense` is incorrect (and should fill with np.nan), but on the other hand I find it also inconsistent that the dtype of the densified array is different from the dtype of the sparse array.

And https://github.com/pandas-dev/pandas/issues/23125 about `astype` is a related issue I think.
> I suspect a primary motivation was supporting sparse integer values with NaN for a fill value. We should investigate what's tested, part of the API, and useful to users.

Do you have an example of how to create such an integer sparse array?

Even with constructing it explicitly with passing integer `sp_values` and `sp_index`, the result has float dtype:

```
In [32]: a = pd.SparseArray([0, 1, 0, 1])

In [33]: a.sp_values
Out[33]: array([1, 1])

In [34]: a.sp_values.dtype
Out[34]: dtype('int64')

In [35]: new_a = pd.SparseArray(a.sp_values, sparse_index=a.sp_index, fill_value=np.nan)

In [36]: new_a.dtype
Out[36]: dtype('float64')
```

So my suspicion is that it was actually not meant to be supported (but of course, only guessing here, as I never used it

---

So for me the main questions is; what is the use case of allowing this? Why allowing the potential confusing, instead of adding a few checks?

And then the "integer with NA" use case you mention is indeed one possible answer. But since (I think, see above) this is a new possibility, and not kept for backwards compatibility, I am personally not sure we should allow it for that reason.

Hmm thanks for testing that out... I thought that additional tests broke, but I could be misremembering.

I do agree that we shouldn't be adding new functionality. If the only thing that breaks is the constructor, i.e.

```python
In [13]: a = pd.SparseArray([1, 0, 0, 1], fill_value=np.nan)

In [14]: a
Out[14]:
[1, 0, 0, 1]
Fill: nan
IntIndex
Indices: array([0, 1, 2, 3], dtype=int32)
```

then I'm OK with not allowing that, because it isn't actually saving any memory in that case.
> Hmm thanks for testing that out... I thought that additional tests broke, but I could be misremembering.

I might be wrong of course, I only tried out some obvious things. Might be good to assure that with the tests.
FYI, I think this will require a change in `DataFrame.to_sparse`. Right now the default `fill_value` is just np.nan. When you have e.g. an int array, you can't have `np.nan` for your fill_value.

So I think we need to change the meaning of `fill_value=None`  in `to_sparse`. Instead of just being `nan`, it will be `na_value_for_dtype(dtype)` on each column.

We'll also I think need to update the API to allow `fill_value` to be a mapping of `{column: fill_value}` when you have a mixed dataframe and want different fill values for each column.
Somewhat xref #39584
take
@TomAugspurger 

Now we have situation when SparseDType keeps both the type of non zero elements and  the VALUE of fill_value.
Why do we need keep fill_value in sparsedtype?
I think this is absolutely no reason to do this.

The crucial thing of sparse array (as I understand, but I'm not a specialist in this topic) is only non zero values and indices.
There is no matter what exactly fill_value we have, zero, nan, or another because we keep in memory only non zero elements array and their positions i.e. indices.

[According to Julia sparsevector docs](https://docs.julialang.org/en/v1/stdlib/SparseArrays/#Sparse-Vector-Storage):

In Julia, sparse vectors have the type SparseVector{Tv,Ti} where Tv is the type of the stored values and Ti the integer type for the indices. The internal representation is as follows:

~~~
struct SparseVector{Tv,Ti<:Integer} <: AbstractSparseVector{Tv,Ti}
    n::Int              # Length of the sparse vector
    nzind::Vector{Ti}   # Indices of stored values
    nzval::Vector{Tv}   # Stored values, typically nonzeros
end
~~~

Looks like more rational to go on this way.


I mentioned this issues in #45126 and it looks like api issue. So I suggest to close this and concentrate on #45126.

Commit message: DEPR: require SparseDtype.fill_value be compatible with SparseDtype.subtype

Patch to review:
diff --git a/doc/source/whatsnew/v2.1.0.rst b/doc/source/whatsnew/v2.1.0.rst
index 84e011bf80da6..f2068ed0e641d 100644
--- a/doc/source/whatsnew/v2.1.0.rst
+++ b/doc/source/whatsnew/v2.1.0.rst
@@ -259,6 +259,7 @@ Deprecations
 - Deprecated unused "closed" and "normalize" keywords in the :class:`DatetimeIndex` constructor (:issue:`52628`)
 - Deprecated unused "closed" keyword in the :class:`TimedeltaIndex` constructor (:issue:`52628`)
 - Deprecated logical operation between two non boolean :class:`Series` with different indexes always coercing the result to bool dtype. In a future version, this will maintain the return type of the inputs. (:issue:`52500`, :issue:`52538`)
+- Deprecated allowing arbitrary ``fill_value`` in :class:`SparseDtype`, in a future version the ``fill_value`` will need to be compatible with the ``dtype.subtype``, either a scalar that can be held by that subtype or ``NaN`` for integer or bool subtypes (:issue:`23124`)
 - Deprecated constructing :class:`SparseArray` from scalar data, pass a sequence instead (:issue:`53039`)
 -
 
diff --git a/pandas/core/arrays/sparse/dtype.py b/pandas/core/arrays/sparse/dtype.py
index dadd161ceeb38..1017cf2112042 100644
--- a/pandas/core/arrays/sparse/dtype.py
+++ b/pandas/core/arrays/sparse/dtype.py
@@ -18,6 +18,7 @@
     ExtensionDtype,
     register_extension_dtype,
 )
+from pandas.core.dtypes.cast import can_hold_element
 from pandas.core.dtypes.common import (
     is_bool_dtype,
     is_object_dtype,
@@ -25,11 +26,15 @@
     is_string_dtype,
     pandas_dtype,
 )
+from pandas.core.dtypes.dtypes import CategoricalDtype
 from pandas.core.dtypes.missing import (
+    is_valid_na_for_dtype,
     isna,
     na_value_for_dtype,
 )
 
+from pandas.core.construction import ensure_wrapped_if_datetimelike
+
 if TYPE_CHECKING:
     from pandas._typing import (
         Dtype,
@@ -161,18 +166,41 @@ def _check_fill_value(self):
             raise ValueError(
                 f"fill_value must be a scalar. Got {self._fill_value} instead"
             )
-        # TODO: Right now we can use Sparse boolean array
-        #       with any fill_value. Here was an attempt
-        #       to allow only 3 value: True, False or nan
-        #       but plenty test has failed.
-        # see pull 44955
-        # if self._is_boolean and not (
-        #    is_bool(self._fill_value) or isna(self._fill_value)
-        # ):
-        #    raise ValueError(
-        #        "fill_value must be True, False or nan "
-        #        f"for boolean type. Got {self._fill_value} instead"
-        #    )
+
+        # GH#23124 require fill_value and subtype to match
+        val = self._fill_value
+        if isna(val):
+            if not is_valid_na_for_dtype(val, self.subtype):
+                warnings.warn(
+                    "Allowing arbitrary scalar fill_value in SparseDtype is "
+                    "deprecated. In a future version, the fill_value must be "
+                    "a valid value for the SparseDtype.subtype.",
+                    FutureWarning,
+                    stacklevel=find_stack_level(),
+                )
+        elif isinstance(self.subtype, CategoricalDtype):
+            # TODO: is this even supported?  It is reached in
+            #  test_dtype_sparse_with_fill_value_not_present_in_data
+            if self.subtype.categories is None or val not in self.subtype.categories:
+                warnings.warn(
+                    "Allowing arbitrary scalar fill_value in SparseDtype is "
+                    "deprecated. In a future version, the fill_value must be "
+                    "a valid value for the SparseDtype.subtype.",
+                    FutureWarning,
+                    stacklevel=find_stack_level(),
+                )
+        else:
+            dummy = np.empty(0, dtype=self.subtype)
+            dummy = ensure_wrapped_if_datetimelike(dummy)
+
+            if not can_hold_element(dummy, val):
+                warnings.warn(
+                    "Allowing arbitrary scalar fill_value in SparseDtype is "
+                    "deprecated. In a future version, the fill_value must be "
+                    "a valid value for the SparseDtype.subtype.",
+                    FutureWarning,
+                    stacklevel=find_stack_level(),
+                )
 
     @property
     def _is_na_fill_value(self) -> bool:
diff --git a/pandas/tests/arrays/sparse/test_array.py b/pandas/tests/arrays/sparse/test_array.py
index 4a0795137f80b..b8effc3eff1d1 100644
--- a/pandas/tests/arrays/sparse/test_array.py
+++ b/pandas/tests/arrays/sparse/test_array.py
@@ -52,33 +52,21 @@ def test_set_fill_value(self):
         arr.fill_value = 2
         assert arr.fill_value == 2
 
-        # TODO: this seems fine? You can construct an integer
-        # sparsearray with NaN fill value, why not update one?
-        # coerces to int
-        # msg = "unable to set fill_value 3\\.1 to int64 dtype"
-        # with pytest.raises(ValueError, match=msg):
-        arr.fill_value = 3.1
+        msg = "Allowing arbitrary scalar fill_value in SparseDtype is deprecated"
+        with tm.assert_produces_warning(FutureWarning, match=msg):
+            arr.fill_value = 3.1
         assert arr.fill_value == 3.1
 
-        # msg = "unable to set fill_value nan to int64 dtype"
-        # with pytest.raises(ValueError, match=msg):
         arr.fill_value = np.nan
         assert np.isnan(arr.fill_value)
 
         arr = SparseArray([True, False, True], fill_value=False, dtype=np.bool_)
         arr.fill_value = True
-        assert arr.fill_value
-
-        # FIXME: don't leave commented-out
-        # coerces to bool
-        # TODO: we can construct an sparse array of bool
-        #      type and use as fill_value any value
-        # msg = "fill_value must be True, False or nan"
-        # with pytest.raises(ValueError, match=msg):
-        #    arr.fill_value = 0
-
-        # msg = "unable to set fill_value nan to bool dtype"
-        # with pytest.raises(ValueError, match=msg):
+        assert arr.fill_value is True
+
+        with tm.assert_produces_warning(FutureWarning, match=msg):
+            arr.fill_value = 0
+
         arr.fill_value = np.nan
         assert np.isnan(arr.fill_value)
 
diff --git a/pandas/tests/arrays/sparse/test_astype.py b/pandas/tests/arrays/sparse/test_astype.py
index 86d69610059b3..446b256e1a186 100644
--- a/pandas/tests/arrays/sparse/test_astype.py
+++ b/pandas/tests/arrays/sparse/test_astype.py
@@ -140,8 +140,13 @@ def test_astype_dt64_to_int64(self):
 def test_dtype_sparse_with_fill_value_not_present_in_data():
     # GH 49987
     df = DataFrame([["a", 0], ["b", 1], ["b", 2]], columns=["A", "B"])
-    result = df["A"].astype(SparseDtype("category", fill_value="c"))
-    expected = Series(
-        ["a", "b", "b"], name="A", dtype=SparseDtype("object", fill_value="c")
-    )
+
+    msg = "Allowing arbitrary scalar fill_value in SparseDtype is deprecated"
+    with tm.assert_produces_warning(FutureWarning, match=msg):
+        dtype1 = SparseDtype("category", fill_value="c")
+
+    result = df["A"].astype(dtype1)
+
+    dtype2 = SparseDtype("object", fill_value="c")
+    expected = Series(["a", "b", "b"], name="A", dtype=dtype2)
     tm.assert_series_equal(result, expected)
diff --git a/pandas/tests/arrays/sparse/test_dtype.py b/pandas/tests/arrays/sparse/test_dtype.py
index 58fedbd3e4231..c893e9760800f 100644
--- a/pandas/tests/arrays/sparse/test_dtype.py
+++ b/pandas/tests/arrays/sparse/test_dtype.py
@@ -1,4 +1,5 @@
 import re
+import warnings
 
 import numpy as np
 import pytest
@@ -67,15 +68,21 @@ def test_nans_equal():
     assert b == a
 
 
-@pytest.mark.parametrize(
-    "a, b",
-    [
+with warnings.catch_warnings():
+    warnings.filterwarnings("ignore")
+
+    tups = [
         (SparseDtype("float64"), SparseDtype("float32")),
         (SparseDtype("float64"), SparseDtype("float64", 0)),
         (SparseDtype("float64"), SparseDtype("datetime64[ns]", np.nan)),
         (SparseDtype(int, pd.NaT), SparseDtype(float, pd.NaT)),
         (SparseDtype("float64"), np.dtype("float64")),
-    ],
+    ]
+
+
+@pytest.mark.parametrize(
+    "a, b",
+    tups,
 )
 def test_not_equal(a, b):
     assert a != b
diff --git a/pandas/tests/reshape/test_get_dummies.py b/pandas/tests/reshape/test_get_dummies.py
index fab9b0a5d1846..6e943863072f1 100644
--- a/pandas/tests/reshape/test_get_dummies.py
+++ b/pandas/tests/reshape/test_get_dummies.py
@@ -57,7 +57,10 @@ def test_get_dummies_basic(self, sparse, dtype):
             dtype=self.effective_dtype(dtype),
         )
         if sparse:
-            expected = expected.apply(SparseArray, fill_value=0.0)
+            if dtype.kind == "b":
+                expected = expected.apply(SparseArray, fill_value=False)
+            else:
+                expected = expected.apply(SparseArray, fill_value=0.0)
         result = get_dummies(s_list, sparse=sparse, dtype=dtype)
         tm.assert_frame_equal(result, expected)
 
@@ -142,7 +145,10 @@ def test_get_dummies_include_na(self, sparse, dtype):
             {"a": [1, 0, 0], "b": [0, 1, 0]}, dtype=self.effective_dtype(dtype)
         )
         if sparse:
-            exp = exp.apply(SparseArray, fill_value=0.0)
+            if dtype.kind == "b":
+                exp = exp.apply(SparseArray, fill_value=False)
+            else:
+                exp = exp.apply(SparseArray, fill_value=0.0)
         tm.assert_frame_equal(res, exp)
 
         # Sparse dataframes do not allow nan labelled columns, see #GH8822
@@ -155,7 +161,10 @@ def test_get_dummies_include_na(self, sparse, dtype):
         # hack (NaN handling in assert_index_equal)
         exp_na.columns = res_na.columns
         if sparse:
-            exp_na = exp_na.apply(SparseArray, fill_value=0.0)
+            if dtype.kind == "b":
+                exp_na = exp_na.apply(SparseArray, fill_value=False)
+            else:
+                exp_na = exp_na.apply(SparseArray, fill_value=0.0)
         tm.assert_frame_equal(res_na, exp_na)
 
         res_just_na = get_dummies([np.nan], dummy_na=True, sparse=sparse, dtype=dtype)
@@ -174,7 +183,7 @@ def test_get_dummies_unicode(self, sparse):
             {"letter_e": [True, False, False], f"letter_{eacute}": [False, True, True]}
         )
         if sparse:
-            exp = exp.apply(SparseArray, fill_value=0)
+            exp = exp.apply(SparseArray, fill_value=False)
         tm.assert_frame_equal(res, exp)
 
     def test_dataframe_dummies_all_obj(self, df, sparse):
@@ -216,7 +225,10 @@ def test_dataframe_dummies_mix_default(self, df, sparse, dtype):
         result = get_dummies(df, sparse=sparse, dtype=dtype)
         if sparse:
             arr = SparseArray
-            typ = SparseDtype(dtype, 0)
+            if dtype.kind == "b":
+                typ = SparseDtype(dtype, False)
+            else:
+                typ = SparseDtype(dtype, 0)
         else:
             arr = np.array
             typ = dtype
@@ -296,7 +308,7 @@ def test_dataframe_dummies_subset(self, df, sparse):
         expected[["C"]] = df[["C"]]
         if sparse:
             cols = ["from_A_a", "from_A_b"]
-            expected[cols] = expected[cols].astype(SparseDtype("bool", 0))
+            expected[cols] = expected[cols].astype(SparseDtype("bool", False))
         tm.assert_frame_equal(result, expected)
 
     def test_dataframe_dummies_prefix_sep(self, df, sparse):
@@ -314,7 +326,7 @@ def test_dataframe_dummies_prefix_sep(self, df, sparse):
         expected = expected[["C", "A..a", "A..b", "B..b", "B..c"]]
         if sparse:
             cols = ["A..a", "A..b", "B..b", "B..c"]
-            expected[cols] = expected[cols].astype(SparseDtype("bool", 0))
+            expected[cols] = expected[cols].astype(SparseDtype("bool", False))
 
         tm.assert_frame_equal(result, expected)
 
@@ -359,7 +371,7 @@ def test_dataframe_dummies_prefix_dict(self, sparse):
         columns = ["from_A_a", "from_A_b", "from_B_b", "from_B_c"]
         expected[columns] = expected[columns].astype(bool)
         if sparse:
-            expected[columns] = expected[columns].astype(SparseDtype("bool", 0))
+            expected[columns] = expected[columns].astype(SparseDtype("bool", False))
 
         tm.assert_frame_equal(result, expected)
 
@@ -371,7 +383,10 @@ def test_dataframe_dummies_with_na(self, df, sparse, dtype):
 
         if sparse:
             arr = SparseArray
-            typ = SparseDtype(dtype, 0)
+            if dtype.kind == "b":
+                typ = SparseDtype(dtype, False)
+            else:
+                typ = SparseDtype(dtype, 0)
         else:
             arr = np.array
             typ = dtype
@@ -399,7 +414,10 @@ def test_dataframe_dummies_with_categorical(self, df, sparse, dtype):
         result = get_dummies(df, sparse=sparse, dtype=dtype).sort_index(axis=1)
         if sparse:
             arr = SparseArray
-            typ = SparseDtype(dtype, 0)
+            if dtype.kind == "b":
+                typ = SparseDtype(dtype, False)
+            else:
+                typ = SparseDtype(dtype, 0)
         else:
             arr = np.array
             typ = dtype
@@ -456,7 +474,7 @@ def test_get_dummies_basic_drop_first(self, sparse):
 
         result = get_dummies(s_list, drop_first=True, sparse=sparse)
         if sparse:
-            expected = expected.apply(SparseArray, fill_value=0)
+            expected = expected.apply(SparseArray, fill_value=False)
         tm.assert_frame_equal(result, expected)
 
         result = get_dummies(s_series, drop_first=True, sparse=sparse)
@@ -490,7 +508,7 @@ def test_get_dummies_basic_drop_first_NA(self, sparse):
         res = get_dummies(s_NA, drop_first=True, sparse=sparse)
         exp = DataFrame({"b": [0, 1, 0]}, dtype=bool)
         if sparse:
-            exp = exp.apply(SparseArray, fill_value=0)
+            exp = exp.apply(SparseArray, fill_value=False)
 
         tm.assert_frame_equal(res, exp)
 
@@ -499,7 +517,7 @@ def test_get_dummies_basic_drop_first_NA(self, sparse):
             ["b", np.nan], axis=1
         )
         if sparse:
-            exp_na = exp_na.apply(SparseArray, fill_value=0)
+            exp_na = exp_na.apply(SparseArray, fill_value=False)
         tm.assert_frame_equal(res_na, exp_na)
 
         res_just_na = get_dummies(
@@ -513,7 +531,7 @@ def test_dataframe_dummies_drop_first(self, df, sparse):
         result = get_dummies(df, drop_first=True, sparse=sparse)
         expected = DataFrame({"A_b": [0, 1, 0], "B_c": [0, 0, 1]}, dtype=bool)
         if sparse:
-            expected = expected.apply(SparseArray, fill_value=0)
+            expected = expected.apply(SparseArray, fill_value=False)
         tm.assert_frame_equal(result, expected)
 
     def test_dataframe_dummies_drop_first_with_categorical(self, df, sparse, dtype):
@@ -632,7 +650,7 @@ def test_get_dummies_duplicate_columns(self, df):
     def test_get_dummies_all_sparse(self):
         df = DataFrame({"A": [1, 2]})
         result = get_dummies(df, columns=["A"], sparse=True)
-        dtype = SparseDtype("bool", 0)
+        dtype = SparseDtype("bool", False)
         expected = DataFrame(
             {
                 "A_1": SparseArray([1, 0], dtype=dtype),


Code review comments:
diff_hunk: @@ -67,15 +68,21 @@ def test_nans_equal():
     assert b == a
 
 
-@pytest.mark.parametrize(
-    "a, b",
-    [
+with warnings.catch_warnings():
+    warnings.filterwarnings("ignore")
path: pandas/tests/arrays/sparse/test_dtype.py
text: @user1:
Could you make this filter more specific?