DEPR/CLN: Clean up to_dense signature deprecations
State: closed

Also deprecate `fill` in `.get_values` because its parameter was being passed to `.to_dense`, which
no longer accepts the `fill` parameter.

xref #14686.


Labels:
name: Deprecate
description: Functionality to remove in pandas
name: Clean

Comments:
Hello @gfyoung! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/core/sparse/array.py`](https://github.com/gfyoung/pandas/blob/21de4f49565831fe44f4c86a0ddad014e5c5a738/pandas/core/sparse/array.py) !

 - There are no PEP8 issues in the file [`pandas/core/sparse/series.py`](https://github.com/gfyoung/pandas/blob/21de4f49565831fe44f4c86a0ddad014e5c5a738/pandas/core/sparse/series.py) !

 - There are no PEP8 issues in the file [`pandas/tests/sparse/series/test_series.py`](https://github.com/gfyoung/pandas/blob/21de4f49565831fe44f4c86a0ddad014e5c5a738/pandas/tests/sparse/series/test_series.py) !

 - There are no PEP8 issues in the file [`pandas/tests/sparse/test_array.py`](https://github.com/gfyoung/pandas/blob/21de4f49565831fe44f4c86a0ddad014e5c5a738/pandas/tests/sparse/test_array.py) !


# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22910?src=pr&el=h1) Report
> Merging [#22910](https://codecov.io/gh/pandas-dev/pandas/pull/22910?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/1546c35dc839b3f6c094ad41d14cda24e9e13a0c?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22910/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22910?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22910      +/-   ##
==========================================
- Coverage   92.19%   92.18%   -0.01%     
==========================================
  Files         169      169              
  Lines       50954    50946       -8     
==========================================
- Hits        46975    46967       -8     
  Misses       3979     3979
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.28% <75%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22910?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/22910/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `92.52% <100%> (-0.03%)` | :arrow_down: |
| [pandas/core/sparse/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/22910/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL3Nlcmllcy5weQ==) | `95.49% <100%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22910?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22910?src=pr&el=footer). Last update [1546c35...bfbae5c](https://codecov.io/gh/pandas-dev/pandas/pull/22910?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

hmm, this might need to go in after @TomAugspurger refactor of Sparse (#22325)
Sounds good.  I'll wait until that's done then.
Looks like Azure is failing on `master` but could be addressed by #23182.
I'm fine with ignoring these failures for now if the code isn't touching window / pyarrow.
@TomAugspurger : These changes don't touch either of those.  That being said, I'm in no rush to merge these, and I see that you are getting close on #23182.

Unless somebody needs these deprecations removed **NOW**, I think I can hold.
I think we could deprecate `get_values` all together? (instead of only deprecating the keyword)
> I think we could deprecate get_values all together? (instead of only deprecating the keyword)

@jorisvandenbossche :

Ah, hence the `TODO` in the original code.  I think `to_dense` makes more sense semantically, so I don't see why we don't do that.  If there aren't any objections, I can do that as well in this PR.
i agree we should deprecate get_values fully
but that means changing where it is used internally

i tried this a while back and it’s not so easy

maybe easier now that SpareArray is here
"in principle" it should not be used internally (outside of sparse itself), as it is not part of the EA interface (or where it is used in sparse specific code, it should be replacable with to_dense) 
but, don't know to what extent this "in principle" is true :)

I think it is certainly worth to check in this PR.

I didn't follow the sparse refactoring too closely, but to what extent are we trying to have a consistent interface for `SparseArray` with respect to `SparseSeries` and `SparseDataFrame` ?

I ask this because although I'm also inclined to deprecate `get_values`, it is of course a completely valid method for the latter two.  The devil's advocate in me thinks then that the interface for sparse object methods becomes a little inconsistent if you have to use `to_dense` for one class but are allowed to use `get_values` for the others.
this looks fine. can you rebase to get this passing. ping on green.
I think we have an issue about deprecating ``.get_values()`` generally, which as I said above we should do (but slightly tricky).
> this looks fine. can you rebase to get this passing. ping on green.

@jreback : I'm waiting on #23182 to be merged.
@gfyoung yep ok. 
> to what extent are we trying to have a consistent interface for SparseArray with respect to SparseSeries and SparseDataFrame ?

There does not need to be a consistency here, since we are considering deprecating SparseSeries/DataFrame alltogether.

I still think it doesn't make sense to only deprecate the keyword if we are actually planning to deprecate the full method. I would directly do that, otherwise you annoy users twice for nothing.

FYI, I've started a exploring a deprecation for SparseDataFrame /
SparseSeries. I think it'll be doable for 0.24.

On Wed, Oct 17, 2018 at 7:04 AM Joris Van den Bossche <
notifications@github.com> wrote:

> to what extent are we trying to have a consistent interface for
> SparseArray with respect to SparseSeries and SparseDataFrame ?
>
> There does not need to be a consistency here, since we are considering
> deprecating SparseSeries/DataFrame alltogether.
>
> I still think it doesn't make sense to only deprecate the feature if we
> are actually planning to deprecate the full method. I would directly do
> that, otherwise you annoy users twice for nothing.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22910#issuecomment-430601481>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIuZgUn9b-20IRUSOIMMwCg4Xz1oKks5ulxzegaJpZM4XBGN8>
> .
>

As @jreback had warned beforehand, there are unfortunately still some issues with trying to deprecate `to_dense` at the moment.  I would prefer to attempt to do this in a fresh PR once this is merged.
I think it was about `get_values` and not `is_dense` (maybe a typo)?
 
But, can you give a bit more details about the issues you encountered? That's useful information to keep track of.
@jorisvandenbossche : There were some errors in the sparse index testing.  I'll try to debug as a follow-up.
@jreback : Addressed all comments, and all is green again!  PTAL.
@gfyoung was looking at the diff now, and was wondering: can't you simply remove the `fill` keyword in `get_values` as well instead of deprecating it? Because you already got a deprecation warning before, since this was simply passed to `to_dense`, which raised the future warning.
@jorisvandenbossche : Ah!  That's a good point!  Let me do that then (given the warning message was not specific to either method).
@jreback @jorisvandenbossche : Comments addressed, and all is green!  PTAL.
thanks @gfyoung