Skip to content

BUG: Fix inconsistency between the shape properties of SparseSeries and SparseArray (#21126) #21198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 31, 2018
Merged

BUG: Fix inconsistency between the shape properties of SparseSeries and SparseArray (#21126) #21198

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
54d5bb8
BUG: Fix inconsistency between the shape properties of SparseSeries a…
nprad May 20, 2018
0538861
Added issue number to test_shape
nprad May 26, 2018
089121a
Merge remote-tracking branch 'upstream/master' into fix_shape
nprad May 27, 2018
c4bbef4
Added what's new entry
nprad May 27, 2018
7cc18c5
Refactored __reduce__, parametrized tests, and reworded release doc
nprad May 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/source/whatsnew/v0.23.1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ Categorical

- Bug in :func:`pandas.util.testing.assert_index_equal` which raised ``AssertionError`` incorrectly, when comparing two :class:`CategoricalIndex` objects with param ``check_categorical=False`` (:issue:`19776`)

Sparse
^^^^^^

- Bug in :attr:`SparseArray.shape` ignores ``fill_values`` (:issue:`21126`)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you expand on this slightly. maybe

Bzug in SparseArray.shape which previouslvy return a shape that only included the dense values.


Conversion
^^^^^^^^^^

Expand Down
8 changes: 7 additions & 1 deletion pandas/core/sparse/array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: why the changes here? Did we have tests that failed without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

object_state[2][1] = super(SparseArray, self).shape
Copy link
Contributor

Choose a reason for hiding this comment

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

this is pretty obtuse, can you make this any more clear

object_state[2] = (tuple(object_state[2]), subclass_state)
return tuple(object_state)

def __setstate__(self, state):
Expand Down Expand Up @@ -339,6 +341,10 @@ def values(self):
output.put(int_index.indices, self)
return output

@property
def shape(self):
return (len(self),)

@property
def sp_values(self):
# caching not an option, leaks memory
Expand Down
11 changes: 11 additions & 0 deletions pandas/tests/sparse/test_array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

out = SparseArray([0, 0, 0, 0, 0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference the github issue here as a comment.

assert out.shape == (5,)

out = SparseArray([])
assert out.shape == (0,)

out = SparseArray([0])
assert out.shape == (1,)

def test_to_dense(self):
vals = np.array([1, np.nan, np.nan, 3, np.nan])
res = SparseArray(vals).to_dense()
Expand Down