ENH: Native conversion from/to scipy.sparse matrix to SparseDataFrame
State: closed

 - [x] closes #4343
 - [x] tests added / passed
 - [X] passes ``git diff upstream/master | flake8 --diff``
 - [X] whatsnew entry


Labels:
name: Enhancement
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Sparse
description: Sparse Data Type

Comments:
The tests require scipy, however scipy is not a pandas dependency. What do?
scipy is already included in many of the test runs e.g. https://github.com/pandas-dev/pandas/blob/master/ci/requirements-2.7.run

you need to use tm._skip_if_no_scipy in your tests (assume u don't need a specific version required)

in code it's a bit more tricky - l will comment on review in a bit
see for example .interpolate where we dispatch to scipy (or raise an error if it's not installed) for some methods
so this API is a bit at odds with what was done in ``SparseSeries.from_coo``, meaning that we have a specific 'type' factory function. We don't actually follow this pattern anywhere else (e.g. even ``DataFrame.from_dict`` is just a small wrapper on the ``DataFrame`` constructor).

So for API consistency, I think it makes sense to change ``SparseSeries.from_coo`` in a similar way.

@wesm IIRC you had some thoughts w.r.t. to this.
I don't have too many opinions on this feature
Never use sparse so also not much opinion, but a question: there already exists a `SparseSeries.from_coo` method. Would it therefore make sense to have a `SparseDataFrame.from_csr` method instead of overloading the constructor?
well @jorisvandenbossche that's exactly the question. just because we have ``.from_coo`` doesn't mean which *should* have specific constructors. In fact we *don't* for *anything* else. So IMHO we should just fold these into the constructor (IOW, leave this PR alone and deprecate and change ``.from_coo``
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15497?src=pr&el=h1) Report
> Merging [#15497](https://codecov.io/gh/pandas-dev/pandas/pull/15497?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/5667a3ad0489815c1239cba785300952c9799000?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `88.33%`.


```diff
@@            Coverage Diff             @@
##           master   #15497      +/-   ##
==========================================
- Coverage   91.02%   90.99%   -0.03%     
==========================================
  Files         143      143              
  Lines       49324    49369      +45     
==========================================
+ Hits        44897    44925      +28     
- Misses       4427     4444      +17
```


| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/15497?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/sparse/array.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL3NwYXJzZS9hcnJheS5weQ==) | `91.46% <100%> (+0.04%)` | :white_check_mark: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `80.92% <40%> (-0.19%)` | :x: |
| [pandas/types/common.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL3R5cGVzL2NvbW1vbi5weQ==) | `93.93% <75%> (-0.53%)` | :x: |
| [pandas/sparse/frame.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL3NwYXJzZS9mcmFtZS5weQ==) | `96.68% <95.45%> (-0.01%)` | :x: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :x: |
| [pandas/tools/merge.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL3Rvb2xzL21lcmdlLnB5) | `91.76% <0%> (-0.35%)` | :x: |
| [pandas/types/cast.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL3R5cGVzL2Nhc3QucHk=) | `85.45% <0%> (-0.21%)` | :x: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.87% <0%> (-0.1%)` | :x: |
| [pandas/core/common.py](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tbW9uLnB5) | `91.3% <0%> (+0.33%)` | :white_check_mark: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15497?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/15497?src=pr&el=footer). Last update [5667a3a...a0f2208](https://codecov.io/gh/pandas-dev/pandas/compare/5667a3ad0489815c1239cba785300952c9799000...a0f22085325e72bb67fe3b98f366ba2b8f47f6a6?src=pr&el=footer&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
> So for API consistency, I think it makes sense to change SparseSeries.from_coo in a similar way.

What way is that? SparseSeries accepting 1×N sparse matrices in the constructor?

> Would it make sense to have a `SparseDataFrame.from_csr` method instead of overloading the constructor?

There are [other types of sparse matrices](https://docs.scipy.org/doc/scipy-0.18.1/reference/sparse.html#sparse-matrix-classes) and while the user could be asked to convert to a specific format, why should they be forced to. Conversion from one sparse format to another is cheap (relative to any operation that makes anything dense). Several spmatrix methods do the conversion to the optimal type for the operation internally as needed. And spmatrix objects are still _de facto_ sparse structures in this stack, so accepting them in the constructor would not be an unexpected functionality. 
> There are other types of sparse matrices and while the user could be asked to convert to a specific format, why should they be forced to. Conversion from one sparse format to another is cheap (relative to any operation that makes anything dense). Several spmatrix methods do the conversion to the optimal type for the operation internally as needed. And spmatrix objects are still de facto sparse structures in this stack, so accepting them in the constructor would not be an unexpected functionality.

so the question is to we have several ``.from_type_of_sparse_conversion`` routines, OR attempt to figure this out in the constructor?

I think it should be consistent 1 way or the other. (e.g. NO implicit construction in the constructor / maybe raise an error actually, with ``.from_*`` methods), or ALL constructions in the constructor.
There are no relevant `from_*` methods on SparseDataFrame currently, only one on SparseSeries, which I don't yet have an opinion on, except that it seems a bit cumbersome and it's not a frame (ndim != 2) whereas a spmatrix is (ndim==2).

As is, we already support _all_ scipy.sparse.spmatrix subclasses through the constructor, so individual per-type `from_*` methods are unnecessary. But I'm happy to add a thin factory method `SparseDataFrame.from_scipy` (or equivalent) that calls the constructor if the consensus is this importantly aids readability. (I'm actually :-1:. SciPy sparse matrices are as ubiquitous in this stack that accepting them in the SparseDataFrame constructor is as natural as accepting ndarrays in DataFrame's.)
> As is, we already support all scipy.sparse.spmatrix subclasses through the constructor, so individual per-type from_* methods are unnecessary. But I'm happy to add a thin factory method SparseDataFrame.from_scipy (or equivalent) that calls the constructor if the consensus is this importantly aids readability. (I'm actually 👎. SciPy sparse matrices are as ubiquitous in this stack that accepting them in the SparseDataFrame constructor is as natural as accepting ndarrays in DataFrame's.)

@kernc so I think you answered my question. If we can unambiguously accept scipy sparse matricies w/o having to have a ``.from_*`` then I am all for it. So would then deprecate``SparseSeries.from_coo``, and move inside the constructor instead.

But deprecate `SparseSeries.from_coo` in favor of what? Accepting a 1×N scipy matrix in the constructor and making it a Series, _and_ accepting a N×M scipy matrix and making it a Series indexed by a MultiIndex?

Basically, whatever but just accepting stuff in the constructor?
> But deprecate SparseSeries.from_coo in favor of what? Accepting a 1×N scipy matrix in the constructor and making it a Series, and accepting a N×M scipy matrix and making it a Series indexed by a MultiIndex?

Simply moving the functionailty *into* the constuctor itself (almost exactly as it is). This is for ``SparseSeries`` (and NOT ``SparseDataFrame``)

IOW, something like


```
class SparseSeries(Series):

     ........

      if is_scipy_matrix(..):
           if is_coo_matrix(....):
                 # do what .from_coo does now
           else:
                raise NotImplentedError('on the todo list')
```


@kernc of course you *may* still want these routines if you need to control the conversion process in some way; though we *maybe* could add those as optional params to the constructor themselves.
you'll want to rebase on master as travis was having some issues
Please give it another look. Besides addressing all comments, I made two major changes since the last iteration, namely:
* checking (testing) that dtypes are preserved when possible,
* the `default_fill_value` when unspecified set to `0` when initialized with a `spmatrix` as this is the implicit missing value in scipy sparse matrices.
just some minor comments actually. lgtm. ping when all green and I think can merge. Will need to follow up to fix SparseSeries anyhow (to accept in the constructor) so can catch any fixes there.
So this is green and kinda works. Feel free to edit at will.
@kernc will have a look tomorrow and merge.
@sinhrks comments?
thanks @kernc great PR!

love for you to take #15634 !
@kernc not a big deal but https://travis-ci.org/pandas-dev/pandas/jobs/209914478

```
.../home/travis/miniconda3/envs/pandas/lib/python3.5/site-packages/scipy/sparse/sputils.py:114: UserWarning: object dtype is not supported by sparse matrices
  warnings.warn("object dtype is not supported by sparse matrices")
......................................................................................................................................................................................................................................................................................................................................................................./home/travis/miniconda3/envs/pandas/lib/python3.5/site-packages/scipy/sparse/sputils.py:114: UserWarning: object dtype is not supported by sparse matrices
  warnings.warn("object dtype is not supported by sparse matrices")
```
should prob skip that test then (or catch warning)
What seems to not be supported are ufuncs and binops upon them. Right, will skip it in #15634 if it can wait till next week.
np thanks
@kernc didn't go through the full reviewing discussion here, but was it ever discussed what the default fill value should be when converting a scipy sparse matrix to SparseDataFrame? 

Because now, it actually "changes" your data due to the default fill value of NaN:

```
In [27]: a = scipy.sparse.coo_matrix([[0., 1.], [2., 0.]])

In [28]: a.toarray()
Out[28]: 
array([[ 0.,  1.],
       [ 2.,  0.]])

In [29]: df = pd.SparseDataFrame(a)

In [30]: df
Out[30]: 
     0    1
0  NaN  1.0
1  2.0  NaN

In [31]: np.array(df)
Out[31]: 
array([[ nan,   1.],
       [  2.,  nan]])
```

I would expect the SparseDataFrame constructor to respect the implicit fill value of a scipy sparse matrix, which is 0.