BUG: Fix inconsistency between the shape properties of SparseSeries and SparseArray (#21126) #21198
Conversation
|
@TomAugspurger I made the changes you suggested. Let me know if there are any changes you need me to make. |
pandas/core/sparse/array.py
Outdated
| @@ -290,7 +290,9 @@ def __reduce__(self): | |||
| """Necessary for making this object picklable""" | |||
| object_state = list(np.ndarray.__reduce__(self)) | |||
| subclass_state = self.fill_value, self.sp_index | |||
| object_state[2] = (object_state[2], subclass_state) | |||
| object_state[2] = list(object_state[2]) | |||
There was a problem hiding this comment.
Just curious: why the changes here? Did we have tests that failed without it?
There was a problem hiding this comment.
Yes, test_pickle was failing.shape is used to pickle objects and the overridden shape does not return the correct value. Therefore, I had to set the shape manually.
pandas/tests/sparse/test_array.py
Outdated
| @@ -454,6 +454,16 @@ def test_values_asarray(self): | |||
| assert_almost_equal(self.arr.to_dense(), self.arr_data) | |||
| assert_almost_equal(self.arr.sp_values, np.asarray(self.arr)) | |||
|
|
|||
| def test_shape(self): | |||
| out = SparseArray([0, 0, 0, 0, 0]) | |||
There was a problem hiding this comment.
Reference the github issue here as a comment.
Codecov Report
@@ Coverage Diff @@
## master #21198 +/- ##
==========================================
+ Coverage 91.84% 91.84% +<.01%
==========================================
Files 153 153
Lines 49505 49543 +38
==========================================
+ Hits 45466 45504 +38
Misses 4039 4039
Continue to review full report at Codecov.
|
|
Thanks for the review! Made the changes. Let me know if there is anything else that needs to be done. |
pandas/core/sparse/array.py
Outdated
| @@ -290,7 +290,9 @@ def __reduce__(self): | |||
| """Necessary for making this object picklable""" | |||
| object_state = list(np.ndarray.__reduce__(self)) | |||
| subclass_state = self.fill_value, self.sp_index | |||
| object_state[2] = (object_state[2], subclass_state) | |||
| object_state[2] = list(object_state[2]) | |||
| object_state[2][1] = super(SparseArray, self).shape | |||
There was a problem hiding this comment.
this is pretty obtuse, can you make this any more clear
pandas/tests/sparse/test_array.py
Outdated
| @@ -454,6 +454,17 @@ def test_values_asarray(self): | |||
| assert_almost_equal(self.arr.to_dense(), self.arr_data) | |||
| assert_almost_equal(self.arr.sp_values, np.asarray(self.arr)) | |||
|
|
|||
| def test_shape(self): | |||
| # GH 21126 | |||
There was a problem hiding this comment.
can you add cases that have a fill_value, can you parameterize the tests. also add a test with an object array as well (e.g. steal cases from the constructor tests)
doc/source/whatsnew/v0.23.1.txt
Outdated
| Sparse | ||
| ^^^^^^ | ||
|
|
||
| - Bug in :attr:`SparseArray.shape` ignores ``fill_values`` (:issue:`21126`) |
There was a problem hiding this comment.
can you expand on this slightly. maybe
Bzug in SparseArray.shape which previouslvy return a shape that only included the dense values.
|
Thanks @jreback for the review! Let me know if this is better |
|
thanks @nprad |
…nd SparseArray (pandas-dev#21126) (pandas-dev#21198) (cherry picked from commit 5348e06)
git diff upstream/master -u -- "*.py" | flake8 --diff