BUG: fix SparseSeries reindex by using Series implementation
State: closed

 - [x] closes #15447
 - [x] tests added / passed
 - [x] passes ``git diff master | flake8 --diff``
 - [x] whatsnew entry

I abunded a bit with the tests for ``.reindex()`` (for ``.loc`` there were already quite a bit).

This is completely independent from the approach in #15448 , which also made sense but on which I will come back in a different PR. In the current PR, I'm just exploiting the fact that sparse ``SparseBlock``s "know what to do", and the ``SparseSeries`` itself has nothing particular to do for reindexing.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Sparse
description: Sparse Data Type

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15461?src=pr&el=h1) Report
> Merging [#15461](https://codecov.io/gh/pandas-dev/pandas/pull/15461?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/04e116851337cd852b4255f8221d9be44829e0e1?src=pr&el=desc) will **increase** coverage by `0.25%`.
> The diff coverage is `100%`.


```diff
@@            Coverage Diff             @@
##           master   #15461      +/-   ##
==========================================
+ Coverage   91.07%   91.33%   +0.25%     
==========================================
  Files         136      137       +1     
  Lines       49165    51133    +1968     
==========================================
+ Hits        44775    46700    +1925     
- Misses       4390     4433      +43
```


| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/15461?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/sparse/series.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL3NwYXJzZS9zZXJpZXMucHk=) | `95.07% <100%> (-0.12%)` | :x: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :x: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `81.11% <0%> (-0.96%)` | :x: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `93.64% <0%> (-0.52%)` | :x: |
| [pandas/tools/merge.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL3Rvb2xzL21lcmdlLnB5) | `91.78% <0%> (-0.35%)` | :x: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.83% <0%> (-0.1%)` | :x: |
| [pandas/core/reshape.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS5weQ==) | `99.15% <0%> (-0.1%)` | :x: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.25% <0%> (-0.07%)` | :x: |
| [pandas/formats/format.py](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?src=pr&el=tree#diff-cGFuZGFzL2Zvcm1hdHMvZm9ybWF0LnB5) | `95.34% <0%> (-0.07%)` | :x: |
| ... and [11 more](https://codecov.io/gh/pandas-dev/pandas/pull/15461?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15461?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/15461?src=pr&el=footer). Last update [04e1168...9084246](https://codecov.io/gh/pandas-dev/pandas/compare/04e116851337cd852b4255f8221d9be44829e0e1...9084246137277efcdcfc836744e4fca4c168a701?el=footer&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
lgtm. just some minor doc-string issues.
can you test with multiple elements as well on the ``.loc`` indexing (unless those tests cases already exist)
``.loc`` with multiple elements is already present ([same test](https://github.com/pandas-dev/pandas/blob/master/pandas/tests/sparse/test_indexing.py#L498)).
(The failed test is apparenty a bug in CircleCI)
lgtm. ping on green (I restarted circleci).


Failed again! My code is apparently so bad it crashes the install.
yeah something happening, though they appear to be working now.
Don't know whether you restarted again, but it's again/still failed.
ping (still stuck)
why don't you push again (you can simply do ``git commit --all --amend -C HEAD`` to generate a new commit. This is the only PR where this is happening.
That didn't help, but rebasing on ``master`` apparently did.
I'ìll push to my personal ``master`` this PR and other commits I have ready (but didn't want to distract you from this); I will try as long as time allows to often rebase on pandas-dev's ``master``. Feel free to cherry-pick when you are interested.
@toobaz that is a pretty poor attitude. fixing something is great. making more bugs is not.
Jeff, I think @toobaz has shown good attitude in the past and put effort in a lot of issues/PRs. Please be both a bit more patient with each other.

If I understand the issue here, the thing is that now the kwargs are passed, and before they were not (so as a result, silently ignored). And those kwargs are not (fully) tested with a SparseSeries. 
But at the same time, @jreback you are OK with the approach in this PR of handling things to `super(SparseSeries, self).reindex(..)` ? And this fixes some bugs? 
If that is the case, let's go forward with this approach, but just not yet pass all the kwargs (just keep ignoring them) until someone adds tests for them? That seems like a simple way forward for this PR?

@toobaz Would you still like to make that small change? (not pass the kwargs, or ony pass those for which you added tests) Then I think we can certainly merge this?


> leave untested only an argument of which (even in the Series case) I can't even find a working example, let alone an unambiguous documentation.

@toobaz about which argument are you talking here exactly?
@toobaz I simply wanted you to raise ``NotImplemented`` on the addtl kwargs. Sure tests for those would be nice. Your fix was fine, but expanded the API to a bunch of non-tested (and undocumented) code that *should* work, but needs validation.
Thanks to both for looking again into this.

The only (as far as I can see from the docs) untested argument is ``level``, so if there are other "_addtl kwargs_" I'm missing, please name them. ``level`` is (as far as I can see) untested also for regular ``Series``, and the documentation is unclear (to me at least). It is _not_ ``NotImplemented``, it is not _tested_ (again, to the best of my ``grep``ping), for ``SparseSeries`` exactly as for ``Series``. For ``SparseSeries``, before, it _was_ a bug. Now, being still not tested, it _might possibly be_ buggy. As stated above, I would be happy to take care of it in another PR, so that this one can finally be merged. That said, if in the meanwhile you want me to raise ``NotImplemented`` for _that specific_ argument - even though _before_ it was not implemented, while now we have _no specific reason to think_ it won't work, sure, I can do it.

(Then my problem is clearly more general. In my PRs, I see more and more often 1) multiple subsequent revisions asking for things which were not previously asked 2) comments showing zero attention to my replies/to the code being discussed 3) the assumption that every missing test is my responsibility, even though ironically **none** of my PRs **ever** extended the pandas API, and all those that fix bugs do provide related tests. I'm not asking you two to open a general discussion on this, I'm just explaining the reason why I will not be able to spend 2 weeks on getting each trivial bug fix merged: the fact that the time I spend on pandas is not paid doesn't necessarily imply I like to waste it. That's it, feel free not to reply, see you in the next PR, which I hope will see more a constructive conversation.)
> level is (as far as I can see) untested also for regular Series, and the documentation is unclear (to me at least).

Small example in the docs: http://pandas.pydata.org/pandas-docs/stable/advanced.html#advanced-reindexing-and-alignment. I think the *behaviour* of reindex(level=) is rather clear (and I also use it regularly), but agree that the docstring explanation is not .. (will open an issue for that). 
Always difficult to find tests in our gigantic test suite, but: there are a few in `tests/test_multilevel.py` ([here](https://github.com/pandas-dev/pandas/blob/09360d80da730008a6a89f38f3780bb1d55f9e25/pandas/tests/test_multilevel.py#L136) and [here](https://github.com/pandas-dev/pandas/blob/09360d80da730008a6a89f38f3780bb1d55f9e25/pandas/tests/test_multilevel.py#L1926)), further also in tests/indexes/test_multi.py.

> For SparseSeries, before, it was a bug. Now, being still not tested, it might possibly be buggy.

I don't know anything of the sparse implementation, so I cannot assess whether it is likely to work out of the box or not. But in any case, this is enabling those keywords (i.e. introducing new functionality that was previously not possible to do), so best to add tests in such a case to assert it is indeed working. 
But let's do that indeed in another PR (and let's open an issue for that), and let's just not pass the kwarg here (leaving the situation exactly how it was) or raise an error (that's the same to me).
> I think the behaviour of reindex(level=) is rather clear

See https://github.com/pandas-dev/pandas/issues/15590 for issue on the docstring for level. But actually, this is not that clear as I thought it was :-) 
I always used it to select a level on the calling object (and therefore I thought the docstring explanation was confusing), but actually it also works on the passed Index .. Will comment about that in #15590
Thanks for the reference to the tests and docs I had missed. Actually, while "exploring" more... I noticed that [my PR _does_ introduce a test](https://github.com/pandas-dev/pandas/pull/15461/files#diff-b44a14f548aa4a78ed3680918b3d0198R572) for ``level``, since before I was asked for it :-) And this happens (and I didn't remember) precisely because I copied the tests from ``Series`` and they just worked. So my ``grep``ping was really bad, but the PR should be good?
thanks @toobaz