Fixes SparseSeries initiated with dictionary raising AttributeError #16960

metllord · 2017-07-15T20:56:55Z

closes BUG: SparseSeries from dict inconsistency #16905
tests added / passed
whatsnew entry

…Issue )

pep8speaks · 2017-07-15T20:56:58Z

Hello @metllord! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 17, 2017 at 14:18 Hours UTC

gfyoung · 2017-07-15T23:01:39Z

doc/source/whatsnew/v0.21.0.txt

 Sparse
 ^^^^^^
-
+- Bug in SparseSeries raises AttributeError when a dicitonary is passed as data (:issue:`16777`)


dicitonary --> dictionary
"passed as" --> "passed in as"

gfyoung · 2017-07-15T23:02:46Z

pandas/tests/sparse/test_series.py

        self.ziseries2 = SparseSeries(arr, index=index, kind='integer',
                                      fill_value=0)
+    def test_constructor_data_input(self):
+        arr = SparseSeries({1: 1})


Can you reference the issue right above this line (e.g. "# see gh-16905)"?

gfyoung · 2017-07-15T23:03:41Z

pandas/tests/sparse/test_series.py

        self.ziseries2 = SparseSeries(arr, index=index, kind='integer',
                                      fill_value=0)
+    def test_constructor_data_input(self):
+        arr = SparseSeries({1: 1})


Notice how you use {1: 1} multiple times. Why don't we refactor that as a variable and then use that variable instead throughout the tests. The same goes for your index=[0, 1, 2]. Set that as a variable.

gfyoung · 2017-07-15T23:04:03Z

pandas/tests/sparse/test_series.py

+        assert arr.count() == len({1: 1})
+
+        arr = SparseSeries(pd.Series({1: 1}, index=[0, 1, 2]))
+        assert arr.count() == pd.Series({1: 1}, index=[0, 1, 2]).count()


I would separate out your pd.Series({1: 1}, index=[0, 1, 2]).count() as a variable like "expected" so that it's a little clearer what the expected output is.

codecov · 2017-07-16T02:24:24Z

Codecov Report

Merging #16960 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16960      +/-   ##
==========================================
- Coverage   90.98%   90.96%   -0.02%     
==========================================
  Files         161      161              
  Lines       49288    49288              
==========================================
- Hits        44846    44837       -9     
- Misses       4442     4451       +9

Flag	Coverage Δ
#multiple	`88.74% <100%> (ø)`	⬆️
#single	`40.2% <100%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/sparse/series.py	`95.07% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.76% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96168ef...19da337. Read the comment docs.

codecov · 2017-07-16T02:24:28Z

Codecov Report

Merging #16960 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16960      +/-   ##
==========================================
- Coverage   90.98%   90.97%   -0.02%     
==========================================
  Files         161      161              
  Lines       49288    49292       +4     
==========================================
- Hits        44846    44844       -2     
- Misses       4442     4448       +6

Flag	Coverage Δ
#multiple	`88.74% <100%> (ø)`	⬆️
#single	`40.19% <100%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/sparse/series.py	`95.06% <100%> (-0.02%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.76% <0%> (-0.1%)`	⬇️
pandas/core/series.py	`94.89% <0%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`86.89% <0%> (ø)`	⬆️
pandas/core/generic.py	`92.29% <0%> (ø)`	⬆️
pandas/core/config_init.py	`96% <0%> (+0.03%)`	⬆️
pandas/plotting/_core.py	`82.72% <0%> (+0.2%)`	⬆️
pandas/plotting/_timeseries.py	`60.73% <0%> (+0.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96168ef...5416762. Read the comment docs.

gfyoung · 2017-07-16T03:03:36Z

pandas/tests/sparse/test_series.py

+        # see gh-16905
+        test_dict = {1: 1}
+        test_index = [0, 1, 2]
+        test_series = pd.Series({1: 1}, index=test_index)


Replace the {1: 1} with test_dict.

gfyoung · 2017-07-16T03:04:18Z

doc/source/whatsnew/v0.21.0.txt

 Sparse
 ^^^^^^
-
+- Bug in SparseSeries raises AttributeError when a dictionary is passed in as data (:issue:`16777`)


Could you add double ticks ("``") around both SparseSeries and `AttributeError`?

gfyoung · 2017-07-16T03:05:14Z

pandas/tests/sparse/test_series.py

+        arr = SparseSeries(test_dict)
+        assert arr.count() == len(test_dict)
+
+        arr = SparseSeries(test_series, index=test_index))


Oops: got an extra right parenthesis there

gfyoung · 2017-07-16T08:45:36Z

@metllord : Ping when green, and we will merge this!

jreback · 2017-07-16T09:25:55Z

pandas/tests/sparse/test_series.py

+        assert arr.count() == len(test_dict)
+
+        arr = SparseSeries(test_series, index=test_index)
+        assert arr.count() == test_series.count()


pls use assert_sp_series_equal

I had to use assert_series_equal since assert_sp_series_equal requires two sparse series, even if check_series_type=False.
This appears to result from line 1523 in util.testing.

I think what you should do is construct your expected result via instantiation as a Series (i.e. pass the dict into the Series constructor). Then pass the Series object into the SparseSeries constructor. That becomes your expected output.

The second test is testing the creation of a SparseSeries from a Series. The SparseSeries constructor takes either a dict or a Series. Aside from class, the SparseSeries should be equal to the original Series.

jreback · 2017-07-16T09:26:26Z

pandas/tests/sparse/test_series.py

+    def test_constructor_data_input(self):
+        # see gh-16905
+        test_dict = {1: 1}
+        test_index = [0, 1, 2]


don't name things with test_

Would _test_dict, etc work? I saw that higher up in the code.

I think @jreback is saying to come up with different names that don't use "test" at all.

jreback · 2017-07-16T09:26:43Z

pandas/tests/sparse/test_series.py

+        test_index = [0, 1, 2]
+        test_series = pd.Series(test_dict, index=test_index)
+
+        arr = SparseSeries(test_dict)


gfyoung · 2017-07-16T10:56:38Z

pandas/tests/sparse/test_series.py


+        arr = SparseSeries({1: 1})
+        assert arr.dtype == np.int64
+


On second look, I don't think you need this. You can remove.

gfyoung · 2017-07-16T10:58:32Z

pandas/tests/sparse/test_series.py

+
+        result = SparseSeries(series, index=index)
+        tm.assert_series_equal(result, series, check_series_type=False)
+


Okay : in light of what you just said, I think you can condense this test a bit and rename as follows:

def test_constructor_dict_input(self): ... series = pd.Series(constructor_dict, index=index) expected = SparseSeries(series, index=index) result = SparseSeries(constructor_dict) tm.assert_sp_series_equal(result, expected)

And delete everything else below it.

Thanks for this. I added this test and did find a case that was not handled correctly. I fixed the code and added tests for all options of SparseSeries creation.

jreback · 2017-07-16T22:16:10Z

pandas/core/sparse/series.py


            elif isinstance(data, (Series, dict)):
+                data = Series(data, index=index)
                if index is None:


hmm, I think you might be able to remove this if index is None bizeness (and simply use data.index if needed below)

see what if anything breaks

I ran the full test suite with this change and everything is working. I also manually ran a bunch of SparseSeries with different inputs (Series, dicts, with/without indexes) and it does work as intended.

gfyoung · 2017-07-16T23:07:48Z

pandas/tests/sparse/test_series.py

        self.ziseries2 = SparseSeries(arr, index=index, kind='integer',
                                      fill_value=0)

+    def test_constructor_data_input(self):


Rename to test_constructor_dict_input and move your "see" comment to right below the definition.

gfyoung · 2017-07-16T23:08:45Z

pandas/tests/sparse/test_series.py

+        result = SparseSeries(constructor_dict)
+        tm.assert_sp_series_equal(result, expected)
+
+        # Series with index and dictionary with no index


Can you explain why this being tested? The result variable is the same as in line 98.

I was testing input with a series, but that is test turned out to be redundant. I removed it from the most recent commit.

gfyoung · 2017-07-17T14:53:49Z

@jreback if you have anything else to add here. Otherwise, LGTM!

jreback · 2017-07-18T23:41:31Z

this duplicates #16906

jreback · 2017-07-19T10:18:01Z

thanks @metllord

Fixes SparseSeries initiated with dictionary raising AttributeError (…

b05ba81

…Issue )

gfyoung added Bug Sparse Sparse Data Type labels Jul 15, 2017

gfyoung reviewed Jul 15, 2017

View reviewed changes

Eric Stein added 2 commits July 15, 2017 21:19

Fixes spelling errors in whatsnew doc

6877159

Refactors test for SparseSeries construction

19da337

gfyoung reviewed Jul 16, 2017

View reviewed changes

fixes syntax error in test_series.py

9869a05

gfyoung approved these changes Jul 16, 2017

View reviewed changes

jreback requested changes Jul 16, 2017

View reviewed changes

Refactors test_series.py based on code review

2d3e318

gfyoung reviewed Jul 16, 2017

View reviewed changes

Eric Stein added 2 commits July 16, 2017 13:51

Fixes bug with index in SparseSeries; tests all construction options

4ce5703

removed unnecessary test for sparceseries

5a4605a

jreback requested changes Jul 16, 2017

View reviewed changes

gfyoung reviewed Jul 16, 2017

View reviewed changes

Removes unecessary code from series.py; cleans up test_series

5416762

gfyoung approved these changes Jul 17, 2017

View reviewed changes

jreback approved these changes Jul 18, 2017

View reviewed changes

jreback mentioned this pull request Jul 18, 2017

BUG: SparseSeries init from dict fixes #16906

Closed

4 tasks

gfyoung added this to the 0.21.0 milestone Jul 19, 2017

jreback merged commit 47e909d into pandas-dev:master Jul 19, 2017

metllord deleted the bugfix/SparseSeries branch July 19, 2017 10:23

kernc mentioned this pull request Jul 21, 2017

TST: Move some Series ctor tests to SharedWithSparse #17050

Merged

4 tasks


		result = SparseSeries(series, index=index)
		tm.assert_series_equal(result, series, check_series_type=False)

Uh oh!

Fixes SparseSeries initiated with dictionary raising AttributeError #16960

Fixes SparseSeries initiated with dictionary raising AttributeError #16960

Uh oh!

Conversation

metllord commented Jul 15, 2017

Uh oh!

pep8speaks commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on July 17, 2017 at 14:18 Hours UTC

Uh oh!

gfyoung Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 16, 2017

Codecov Report

Uh oh!

codecov bot commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfyoung commented Jul 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfyoung commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback commented Jul 18, 2017

Uh oh!

jreback commented Jul 19, 2017

Uh oh!

Reviewers

Assignees

Labels

pep8speaks commented Jul 15, 2017 •

edited

Loading

gfyoung Jul 15, 2017 •

edited

Loading

codecov bot commented Jul 16, 2017 •

edited

Loading

gfyoung commented Jul 17, 2017 •

edited

Loading