BUG: Slicing subclasses of SparseDataFrames. #13787

sstanovnik · 2016-07-25T15:11:04Z

1 test added & passed
passes git diff upstream/master | flake8 --diff

This changes SparseDataFrame to use proper subclassing functionality so slicing of subclasses of SparseDataFrame works. Example of a failure that this PR fixes:

import pandas as pd

class DenseSubclassDF(pd.DataFrame):
    _constructor = property(lambda self: DenseSubclassDF)
    _constructor_sliced = property(lambda self: DenseSubclassS)

class DenseSubclassS(pd.Series):
    _constructor = property(lambda self: DenseSubclassS)
    _constructor_expanddim = property(lambda self: DenseSubclassDF)

class SparseSubclassDF(pd.SparseDataFrame):
    _constructor = property(lambda self: SparseSubclassDF)
    _constructor_sliced = property(lambda self: SparseSubclassS)

class SparseSubclassS(pd.SparseSeries):
    _constructor = property(lambda self: SparseSubclassS)
    _constructor_expanddim = property(lambda self: SparseSubclassDF)

ddf = DenseSubclassDF([[1,2,3], [4,5,6], [7,8,9]])
sdf = SparseSubclassDF([[1,2,3], [4,5,6], [7,8,9]])

print(type(ddf.iloc[0]))  # <class '__main__.DenseSubclassS'>
print(type(ddf.iloc[:2]))  # <class '__main__.DenseSubclassDF'>
print(type(ddf[:2]))  # <class '__main__.DenseSubclassDF'>

# sparse doesn't preserve types
print(type(sdf.iloc[0]))  # <class '__main__.SparseSubclassS'>
print(type(sdf.iloc[:2]))  # <class 'pandas.sparse.frame.SparseDataFrame'>
print(type(sdf[:2]))  # <class 'pandas.sparse.frame.SparseDataFrame'>

jreback · 2016-07-25T21:35:39Z

pandas/sparse/frame.py

@@ -373,7 +373,9 @@ def _slice(self, slobj, axis=0, kind=None):
            new_index = self.index
            new_columns = self.columns[slobj]

-        return self.reindex(index=new_index, columns=new_columns)
+        return self._constructor(data=self.reindex(index=new_index,


try not to use \, instead:

return self._constructor( data=self.reindex(index=new_index, columns=new_columns)).__finalize__(self)

can't we fix .reindex to return subclass?

jreback · 2016-07-25T21:38:29Z

is there a specific issue for this?

@sinhrks pls have a look

codecov-io · 2016-07-25T22:43:04Z

Current coverage is 85.25% (diff: 100%)

Merging #13787 into master will increase coverage by <.01%

@@             master     #13787   diff @@
==========================================
  Files           140        140          
  Lines         50455      50471    +16   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43014      43031    +17   
+ Misses         7441       7440     -1   
  Partials          0          0

Powered by Codecov. Last update 59f2557...a7fb24f

sstanovnik · 2016-07-26T10:28:37Z

Thank you for your comments, I made changes according to them:

.reindex didn't need to be adapted, the problem was _reindex_columns which explicitly instantiated a SparseDataFrame. I changed all such calls to use _constructor.
The thing to check in testing.py is that I added obj as a kwarg to panel checks to conform to what the series and frame ones look like. Previously, obj was referenced in them but the one that was used was a leftover from the for loop around line 2600.
There is no issue that handles this problem, the closest one is the sparse master issue BUG: Sparse master issue #10627. Reference that one or create a new one?

sinhrks · 2016-07-26T10:35:24Z

pandas/util/testing.py

+
+    Parameters
+    ----------
+    left : DataFrame


nice docstring:) SparseDataFrame.

sinhrks · 2016-07-26T10:47:46Z

@sstanovnik thx for update! i linked the PR from #10627, so new issue is not needed unless there is something which is not fixid in this PR.

sstanovnik · 2016-07-26T11:13:38Z

Should I also squash the commits?

sinhrks · 2016-07-26T11:37:15Z

not mandatory on your side. will be done during merge.

sstanovnik · 2016-07-26T11:43:32Z

Typo fixed, rebased.

sstanovnik · 2016-07-27T09:03:44Z

Two old pickling tests failed: see this build. I didn't see a nice way out, so I made an exception for the deprecated SparseTimeSeries. This may not be what you want and needs review.

sinhrks · 2016-07-27T10:06:06Z

@sstanovnik yeah SparseTimeSeries is deprecated, you can switch validation option depending on the version, like:

https://github.com/pydata/pandas/blob/master/pandas/io/tests/test_pickle.py#L120

jreback · 2016-07-27T10:29:57Z

pandas/tests/frame/test_subclass.py

@@ -210,3 +210,25 @@ def test_subclass_align_combinations(self):
        tm.assert_series_equal(res1, exp2)
        tm.assertIsInstance(res2, tm.SubclassedDataFrame)
        tm.assert_frame_equal(res2, exp1)
+
+    def test_subclass_sparse_slice(self):


we have lots of changes, but want to see if we can have a test for each, can you audit these changes and add tests if needed.

sstanovnik · 2016-07-28T09:24:13Z

I added additional tests to check other subclassing changes I made.

jreback · 2016-07-29T00:30:47Z

lgtm; though I think the read_pickle assertion can be done in a clearer way (use sep function). @sinhrks ?

sinhrks · 2016-07-29T00:44:09Z

pandas/io/tests/test_pickle.py

@@ -44,8 +44,15 @@ def compare_element(self, result, expected, typ, version=None):
            return

        if typ.startswith('sp_'):
+            # SparseTimeSeries deprecated in 0.17.0
+            if (typ == "sp_series" and version and


test_pickle can use object-specific method if it is defined. Defining compare_sp_series_ts (like other compare_xxx method) should allow test_pickle to use the method for sparse time series. Current impl skips type check of all sparse series.

NOTE: method name is derived from pickle key:

https://github.com/pydata/pandas/blob/master/pandas/io/tests/generate_legacy_storage_files.py#L157

jreback · 2016-07-29T11:49:00Z

@sstanovnik looks good. @sinhrks anything further?

sinhrks · 2016-07-29T11:51:39Z

None. thanks for all the effort, @sstanovnik !

jreback · 2016-08-01T10:50:11Z

@sstanovnik lgtm. pls add a whatsnew note (put in API changes). ping when green.

Use proper subclassing behaviour so subclasses work properly: this fixes an issue where a multi-element slice of a subclass of SparseDataFrame returned the SparseDataFrame type instead of the subclass type.

This should fix subclassing issues.

sstanovnik · 2016-08-01T13:01:50Z

ping

jreback · 2016-08-02T10:42:11Z

thanks @sstanovnik

sstanovnik changed the title ~~[FIX] Slicing subclasses of SparseDataFrames.~~ BUG: Slicing subclasses of SparseDataFrames. Jul 25, 2016

jreback reviewed Jul 25, 2016
View reviewed changes

jreback added Sparse Sparse Data Type Compat pandas objects compatability with Numpy or Python functions labels Jul 25, 2016

sinhrks reviewed Jul 26, 2016
View reviewed changes

pandas/util/testing.py

Parameters

----------

left : DataFrame

Copy link

Member

sinhrks Jul 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice docstring:) SparseDataFrame.

kawochen mentioned this pull request Jul 26, 2016

BUG: Sparse master issue #10627

Closed

18 tasks

jreback reviewed Jul 27, 2016
View reviewed changes

sinhrks reviewed Jul 29, 2016
View reviewed changes

jreback added this to the 0.19.0 milestone Jul 29, 2016

sstanovnik added 3 commits August 1, 2016 13:03

BUG: Fix slicing subclasses of SparseDataFrames.

ddc2874

Use proper subclassing behaviour so subclasses work properly: this fixes an issue where a multi-element slice of a subclass of SparseDataFrame returned the SparseDataFrame type instead of the subclass type.

Formatting changes.

7c10bb5

Add type checks to sparse object comparisons.

cdb7c8c

sstanovnik added 9 commits August 1, 2016 13:03

Modify the new test to compare complete objects.

140e4ad

Use _constructor when instantiating SparseDataFrames.

1d4e9da

This should fix subclassing issues.

Lint corrections.

4e31303

Further SparseSeries subclass fixes.

0387303

Proper test parameter override, travis soothing.

d73ec68

Additional sparse subclass tests.

436a06d

Properer test assertion override.

0b74034

Changed the override from 0.17.0 to 0.12.0.

8d2ff79

Add a whatsnew note.

a7fb24f

jreback closed this in a7f7e1d Aug 2, 2016

jreback mentioned this pull request Aug 2, 2016

ENH: add sparse op for int64 dtypes #13848

Closed

4 tasks

sstanovnik deleted the fix-sparse-slice branch August 4, 2016 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Slicing subclasses of SparseDataFrames. #13787

BUG: Slicing subclasses of SparseDataFrames. #13787

sstanovnik commented Jul 25, 2016 •

edited

Loading

jreback Jul 25, 2016

sinhrks Jul 25, 2016

jreback commented Jul 25, 2016

codecov-io commented Jul 25, 2016 •

edited

Loading

sstanovnik commented Jul 26, 2016

sinhrks Jul 26, 2016

sinhrks commented Jul 26, 2016 •

edited

Loading

sstanovnik commented Jul 26, 2016

sinhrks commented Jul 26, 2016

sstanovnik commented Jul 26, 2016

sstanovnik commented Jul 27, 2016

sinhrks commented Jul 27, 2016

jreback Jul 27, 2016

sstanovnik commented Jul 28, 2016

jreback commented Jul 29, 2016

sinhrks Jul 29, 2016

jreback commented Jul 29, 2016

sinhrks commented Jul 29, 2016

jreback commented Aug 1, 2016

sstanovnik commented Aug 1, 2016

jreback commented Aug 2, 2016

BUG: Slicing subclasses of SparseDataFrames. #13787

BUG: Slicing subclasses of SparseDataFrames. #13787

Conversation

sstanovnik commented Jul 25, 2016 • edited Loading

jreback Jul 25, 2016

Choose a reason for hiding this comment

sinhrks Jul 25, 2016

Choose a reason for hiding this comment

jreback commented Jul 25, 2016

codecov-io commented Jul 25, 2016 • edited Loading

Current coverage is 85.25% (diff: 100%)

sstanovnik commented Jul 26, 2016

sinhrks Jul 26, 2016

Choose a reason for hiding this comment

sinhrks commented Jul 26, 2016 • edited Loading

sstanovnik commented Jul 26, 2016

sinhrks commented Jul 26, 2016

sstanovnik commented Jul 26, 2016

sstanovnik commented Jul 27, 2016

sinhrks commented Jul 27, 2016

jreback Jul 27, 2016

Choose a reason for hiding this comment

sstanovnik commented Jul 28, 2016

jreback commented Jul 29, 2016

sinhrks Jul 29, 2016

Choose a reason for hiding this comment

jreback commented Jul 29, 2016

sinhrks commented Jul 29, 2016

jreback commented Aug 1, 2016

sstanovnik commented Aug 1, 2016

jreback commented Aug 2, 2016

sstanovnik commented Jul 25, 2016 •

edited

Loading

codecov-io commented Jul 25, 2016 •

edited

Loading

sinhrks commented Jul 26, 2016 •

edited

Loading