BUG: Should not raise error in concatenating Series with numpy scalar and tuple names (GH21015) #21132

KalyanGokhale · 2018-05-19T13:26:57Z

closes BUG: error in concatting Series with numpy scalar / tuple names #21015
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Does not raise error in concatting Series with numpy scalar and tuple names

codecov · 2018-05-19T14:08:06Z

Codecov Report

Merging #21132 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #21132      +/-   ##
==========================================
+ Coverage   91.83%   91.84%   +<.01%     
==========================================
  Files         153      153              
  Lines       49498    49502       +4     
==========================================
+ Hits        45458    45463       +5     
+ Misses       4040     4039       -1

Flag	Coverage Δ
#multiple	`90.23% <100%> (ø)`	⬆️
#single	`41.88% <40%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/common.py	`92.09% <100%> (+0.08%)`	⬆️
pandas/core/reshape/reshape.py	`99.78% <0%> (-0.22%)`	⬇️
pandas/core/frame.py	`97.23% <0%> (ø)`	⬆️
pandas/util/testing.py	`84.81% <0%> (+0.21%)`	⬆️

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 e033c06...8aa7e05. Read the comment docs.

Changed to negative test for bool or np.bool_

WillAyd

Thanks for the PR! Comments below - can you also add a whatsnew note?

WillAyd · 2018-05-19T16:32:50Z

pandas/core/common.py

@@ -55,7 +55,11 @@ def flatten(l):
 def _consensus_name_attr(objs):
    name = objs[0].name
    for obj in objs[1:]:
-        if obj.name != name:
+        name_check = (obj.name != name)
+        if not isinstance(name_check, (bool, np.bool_)):


Can you use a try...except block here to catch the ValueError instead of type introspection?

@WillAyd thanks - will make the changes. Should I add whatsnew note to v0.23.1?

Yes that works

Changed to try except block, instead of type introspection

jreback · 2018-05-19T17:55:16Z

pandas/core/common.py

-        if obj.name != name:
+        try:
+            if obj.name != name:
+                return None


do like

try: if obj.name != name: name = None except ValueError: name = None return name

@jreback Thanks - will make the changes

jreback · 2018-05-19T17:55:40Z

doc/source/whatsnew/v0.23.1.txt

@@ -82,7 +82,7 @@ Plotting
 Reshaping
 ^^^^^^^^^

-
+- Bug in :func:`concat` where error was raised in concatenating Series with numpy scalar and tuple names (:issue:`21015`)


double backticks around Series, is this only a numpy scalar or any scalar?

only numpy scalar - as it returns an array on comparison with tuple

ok that's fine (but add in an integer in the test as well below)

jreback · 2018-05-19T17:56:17Z

pandas/tests/reshape/test_concat.py

@@ -2350,6 +2350,21 @@ def test_concat_datetime_timezone(self):

        tm.assert_frame_equal(result, expected)

+    def test_concat_series_name_npscalar_tuple(self):


this doesn't need to be in the class, it can just be a module level test (e.g. de-dent)

jreback · 2018-05-19T17:56:37Z

pandas/tests/reshape/test_concat.py

@@ -2350,6 +2350,21 @@ def test_concat_datetime_timezone(self):

        tm.assert_frame_equal(result, expected)

+    def test_concat_series_name_npscalar_tuple(self):


parameterize this on a np.int64 and an int

done - took the test out of class, parameterised as suggested, also actually removed the test case which took a row from DataFrame

jreback · 2018-05-19T17:58:51Z

pandas/tests/reshape/test_concat.py

+        expected = pd.Series({'a': 1.5})
+        tm.assert_series_equal(result, expected)
+
+        df1 = pd.DataFrame([[1, 2], [3, 4]], columns=['a', 'b'], index=[0, 1])


this is very weird to construct a DataFrame to then take a row from it. Just directly construct the series.

will update - had added the original test case from the issue #21015

@jreback

made changes suggested by @jreback on 19May

@jreback

made changes suggested by @jreback on 19May

WillAyd

Very minor style nits but otherwise lgtm

WillAyd · 2018-05-20T01:50:46Z

pandas/tests/reshape/test_concat.py

@@ -2487,3 +2487,14 @@ def test_concat_aligned_sort_does_not_raise():
                            columns=[1, 'a'])
    result = pd.concat([df, df], ignore_index=True, sort=True)
    tm.assert_frame_equal(result, expected)
+
+
+@pytest.mark.parametrize("s1name, s2name", [(np.int64(190), (43, 0)),


No space between s1name and s2name here

WillAyd · 2018-05-20T01:51:15Z

pandas/tests/reshape/test_concat.py

+
+
+@pytest.mark.parametrize("s1name, s2name", [(np.int64(190), (43, 0)),
+                                            (190, (43, 0))])


Could probably push the tuple on the above line down here - would make the indentation more readable

done. Thanks

@WillAyd

made minor style changes to test file suggested by @WillAyd

@WillAyd

made minor style changes to test file suggested by @WillAyd

WillAyd · 2018-05-20T02:51:26Z

pandas/tests/reshape/test_concat.py

+
+
+@pytest.mark.parametrize("s1name,s2name", [
+                        (np.int64(190), (43, 0)), (190, (43, 0))])


The open bracket is correct but the line below is now over-indented - make sure you set that back accordingly

done - sorry for so many iterations

WillAyd · 2018-05-20T02:53:00Z

doc/source/whatsnew/v0.23.1.txt

@@ -82,7 +82,7 @@ Plotting
 Reshaping
 ^^^^^^^^^

-
+- Bug in :func:`concat` where error was raised in concatenating ``Series`` with numpy scalar and tuple names (:issue:`21015`)


Let's replace ``Series`` with :class:`Series`

@WillAyd

made minor style changes to whatsnew file suggested by @WillAyd

@WillAyd

made minor style changes to test file suggested by @WillAyd

jreback · 2018-05-21T10:41:26Z

thanks @KalyanGokhale

… and tuple names (GH21015) (pandas-dev#21132) (cherry picked from commit e80cc43)

… and tuple names (GH21015) (#21132) (cherry picked from commit e80cc43)

… and tuple names (GH21015) (pandas-dev#21132)

KalyanGokhale added 2 commits May 19, 2018 18:46

GH21015

13f00df

Does not raise error in concatting Series with numpy scalar and tuple names

GH21015

354eb2d

Does not raise error in concatting Series with numpy scalar and tuple names

GH21015

613ab4e

Changed to negative test for bool or np.bool_

WillAyd requested changes May 19, 2018

View reviewed changes

WillAyd added the Bug label May 19, 2018

WillAyd added this to the 0.23.1 milestone May 19, 2018

GH21015

3e84f9c

Changed to try except block, instead of type introspection

jreback requested changes May 19, 2018

View reviewed changes

jreback added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label May 19, 2018

KalyanGokhale added 2 commits May 20, 2018 00:58

GH21015

35b664f

made changes suggested by @jreback on 19May

GH21015

7ec0135

made changes suggested by @jreback on 19May

WillAyd requested changes May 20, 2018

View reviewed changes

KalyanGokhale added 2 commits May 20, 2018 07:47

GH21015

345c084

made minor style changes to test file suggested by @WillAyd

GH21015

21d834e

made minor style changes to test file suggested by @WillAyd

WillAyd requested changes May 20, 2018

View reviewed changes

KalyanGokhale added 2 commits May 20, 2018 08:25

GH21015

ba8abba

made minor style changes to whatsnew file suggested by @WillAyd

GH21015

8aa7e05

made minor style changes to test file suggested by @WillAyd

WillAyd approved these changes May 20, 2018

View reviewed changes

jreback added the Needs Backport label May 21, 2018

jreback approved these changes May 21, 2018

View reviewed changes

jreback merged commit e80cc43 into pandas-dev:master May 21, 2018

KalyanGokhale deleted the concat-series branch May 21, 2018 12:01

jorisvandenbossche removed the Needs Backport label Jun 8, 2018

jorisvandenbossche pushed a commit to jorisvandenbossche/pandas that referenced this pull request Jun 8, 2018

BUG: Should not raise error in concatenating Series with numpy scalar…

e16214c

… and tuple names (GH21015) (pandas-dev#21132) (cherry picked from commit e80cc43)

jorisvandenbossche pushed a commit that referenced this pull request Jun 9, 2018

BUG: Should not raise error in concatenating Series with numpy scalar…

04f5a6e

… and tuple names (GH21015) (#21132) (cherry picked from commit e80cc43)

david-liu-brattle-1 pushed a commit to david-liu-brattle-1/pandas that referenced this pull request Jun 18, 2018

BUG: Should not raise error in concatenating Series with numpy scalar…

72f2007

… and tuple names (GH21015) (pandas-dev#21132)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Should not raise error in concatenating Series with numpy scalar and tuple names (GH21015) #21132

BUG: Should not raise error in concatenating Series with numpy scalar and tuple names (GH21015) #21132

KalyanGokhale commented May 19, 2018 •

edited

Loading

codecov bot commented May 19, 2018 •

edited

Loading

WillAyd left a comment

WillAyd May 19, 2018

KalyanGokhale May 19, 2018

WillAyd May 19, 2018

KalyanGokhale May 19, 2018

jreback May 19, 2018

KalyanGokhale May 19, 2018

KalyanGokhale May 19, 2018

jreback May 19, 2018

KalyanGokhale May 19, 2018

jreback May 19, 2018

jreback May 19, 2018

jreback May 19, 2018

KalyanGokhale May 19, 2018

jreback May 19, 2018

KalyanGokhale May 19, 2018

WillAyd left a comment

WillAyd May 20, 2018

KalyanGokhale May 20, 2018

WillAyd May 20, 2018

KalyanGokhale May 20, 2018

WillAyd May 20, 2018

KalyanGokhale May 20, 2018

WillAyd May 20, 2018

KalyanGokhale May 20, 2018

jreback commented May 21, 2018

		@@ -2350,6 +2350,21 @@ def test_concat_datetime_timezone(self):

		tm.assert_frame_equal(result, expected)

		def test_concat_series_name_npscalar_tuple(self):



		@pytest.mark.parametrize("s1name, s2name", [(np.int64(190), (43, 0)),
		(190, (43, 0))])



		@pytest.mark.parametrize("s1name,s2name", [
		(np.int64(190), (43, 0)), (190, (43, 0))])

BUG: Should not raise error in concatenating Series with numpy scalar and tuple names (GH21015) #21132

BUG: Should not raise error in concatenating Series with numpy scalar and tuple names (GH21015) #21132

Conversation

KalyanGokhale commented May 19, 2018 • edited Loading

codecov bot commented May 19, 2018 • edited Loading

Codecov Report

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented May 21, 2018

KalyanGokhale commented May 19, 2018 •

edited

Loading

codecov bot commented May 19, 2018 •

edited

Loading