BUG: merging an Integer EA rasises #23262

makbigc · 2018-10-21T14:27:16Z

closes BUG: merging on an Integer EA raises #23020
1 test added
whatsnew entry

pep8speaks · 2018-10-21T14:27:20Z

Hello @makbigc! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/reshape/merge.py !
There are no PEP8 issues in the file pandas/tests/reshape/merge/test_merge.py !

jreback · 2018-10-26T00:52:42Z

doc/source/whatsnew/v0.24.0.txt

@@ -1109,6 +1109,7 @@ Reshaping
 - Bug in :func:`merge_asof` when merging on float values within defined tolerance (:issue:`22981`)
 - Bug in :func:`pandas.concat` when concatenating a multicolumn DataFrame with tz-aware data against a DataFrame with a different number of columns (:issue`22796`)
 - Bug in :func:`merge_asof` where confusing error message raised when attempting to merge with missing values (:issue:`23189`)
+- Bug in :func:`pandas.merge` when merging on an Integer extension array (:issue:`23020`)


just move this issue number to the IntegerArray section as that is first announcing IA

jreback · 2018-10-26T00:53:38Z

pandas/tests/reshape/merge/test_merge.py

@@ -1842,6 +1842,16 @@ def test_merging_with_bool_or_int_cateorical_column(self, category_column,
            CDT(categories, ordered=ordered))
        assert_frame_equal(expected, result)

+    def test_merge_on_int_array(self):


move to pandas/tests/extension/base/reshape (and use the fixtures)

jreback · 2018-10-26T00:57:53Z

pandas/core/reshape/merge.py

@@ -1533,6 +1534,10 @@ def _factorize_keys(lk, rk, sort=True):

        lk = ensure_int64(lk.codes)
        rk = ensure_int64(rk)
+    elif is_extension_array_dtype(lk) and is_extension_array_dtype(rk):
+        klass = libhashtable.Factorizer


I think this is actually more complicated here

elif is_extension_array_dtype(lk) and is_extension_array_dtype(rk) and lk.dtype == rk.dtype: klass = ... lk, _ = lk._values_for_factorize() rk, _ = rk._values_for_factorize()

and actually you can do this before any of the other if statements. That way if the _values_for_factorize returns an int64 array then it will just work.

@makbigc did you try @jreback's suggesting of factorizing the EAs before getting into the main if block?

jreback · 2018-11-03T14:29:15Z

can you merge master and update

makbigc · 2018-11-04T15:40:39Z

@jreback before the push, I already rebased my code. But all checks have failed. Did my change break the checks? Or rebasing hasn't get the code to the latest, has it? Please help.

jreback · 2018-11-04T15:45:43Z

doc/source/whatsnew/v0.24.0.txt

@@ -1332,6 +1332,12 @@ Sparse
 - Bug in unary inversion operator (``~``) on a ``SparseSeries`` with boolean values. The performance of this has also been improved (:issue:`22835`)
 - Bug in :meth:`SparseArary.unique` not returning the unique values (:issue:`19595`)

+IntegerArray


just add this onto the big section on IntegerArray which already exists

jreback · 2018-11-04T15:46:39Z

pandas/tests/extension/base/reshaping.py

@@ -170,3 +170,11 @@ def test_merge(self, data, na_value):
                 [data[0], data[0], data[1], data[2], na_value],
                 dtype=data.dtype)})
        self.assert_frame_equal(res, exp[['ext', 'int1', 'key', 'int2']])
+
+    def test_merge_on_int_array(self, df_merge_on_int_array):


this needs to be generic, see the test_merge example. it should work for any array type.

jreback · 2018-11-04T15:46:47Z

pandas/tests/extension/conftest.py

@@ -104,3 +106,9 @@ def data_for_grouping():
 def box_in_series(request):
    """Whether to box the data in a Series"""
    return request.param
+
+


this is not needed

TomAugspurger · 2018-11-07T15:28:10Z

@makbigc can you clarify

But all checks have failed?

It'd be easier to help if you push your changes so that we can see what you have.

TomAugspurger · 2018-11-07T15:28:20Z

Could you also address @jreback's comments?

makbigc · 2018-11-08T14:03:31Z

Please give me some more time. I am working on it.

TomAugspurger · 2018-11-08T14:24:39Z

Thanks. No rush for now, if you think you'll have some time by next week that'd be great.

…

On Thu, Nov 8, 2018 at 8:04 AM Mak Sze Chun ***@***.***> wrote: Reopened #23262 <#23262>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#23262 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHItIaWxb7_FAxylHQu4eD9wslHz0Yks5utDnYgaJpZM4XyfK9> .

doc/source/whatsnew/v0.24.0.rst

pandas/core/reshape/merge.py

pandas/tests/extension/base/reshaping.py

jreback · 2018-12-03T02:02:58Z

@makbigc can you merge master.

codecov · 2018-12-05T15:55:15Z

Codecov Report

Merging #23262 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23262      +/-   ##
==========================================
- Coverage    92.2%    92.2%   -0.01%     
==========================================
  Files         162      162              
  Lines       51714    51722       +8     
==========================================
+ Hits        47682    47689       +7     
- Misses       4032     4033       +1

Flag	Coverage Δ
#multiple	`90.6% <100%> (ø)`	⬆️
#single	`43.01% <0%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/reshape/merge.py	`94.35% <100%> (+0.06%)`	⬆️
pandas/io/json/json.py	`92.61% <0%> (-0.48%)`	⬇️
pandas/util/testing.py	`87.51% <0%> (+0.09%)`	⬆️

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 9f2c716...b8ed3e9. Read the comment docs.

codecov · 2018-12-05T15:55:16Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #23262      +/-   ##
==========================================
+ Coverage   92.28%   92.28%   +<.01%     
==========================================
  Files         162      162              
  Lines       51831    51837       +6     
==========================================
+ Hits        47830    47837       +7     
+ Misses       4001     4000       -1

Flag	Coverage Δ
#multiple	`90.68% <100%> (ø)`	⬆️
#single	`43% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/reshape/merge.py	`94.3% <100%> (+0.01%)`	⬆️
pandas/core/indexes/period.py	`93.09% <0%> (+0.03%)`	⬆️
pandas/core/reshape/tile.py	`94.82% <0%> (+0.06%)`	⬆️
pandas/util/testing.py	`87.58% <0%> (+0.09%)`	⬆️

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 216986d...6bae6ca. Read the comment docs.

makbigc · 2018-12-06T13:57:32Z

@jreback rebased. Please tell me if anything needs to be done.

TomAugspurger · 2018-12-06T17:47:37Z

There's a linting error: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=4750

2018-12-05T15:26:49.5779150Z Check import format using isort
2018-12-05T15:26:55.4954327Z ERROR: /home/vsts/work/1/s/pandas/core/reshape/merge.py Imports are incorrectly sorted.
2018-12-05T15:26:55.5184554Z Check import format using isort DONE

doc/source/whatsnew/v0.24.0.rst

pandas/core/reshape/merge.py

pandas/tests/extension/base/reshaping.py

makbigc · 2018-12-16T16:20:02Z

The test is rewritten. So the EA is the merging key, not like the previous test in which integer array is the merging key for every cases.
The old test is placed as a special case in the tests/reshape/merge/test_merge.py.
Would you give me some hints to pass azure test? It seems the new test doesn't work with python 2.7.15.
Thanks.

pandas/core/arrays/period.py

pandas/core/arrays/sparse.py

TomAugspurger · 2018-12-16T20:11:13Z

pandas/core/reshape/merge.py

@@ -1533,6 +1534,10 @@ def _factorize_keys(lk, rk, sort=True):

        lk = ensure_int64(lk.codes)
        rk = ensure_int64(rk)
+    elif is_extension_array_dtype(lk) and is_extension_array_dtype(rk):
+        klass = libhashtable.Factorizer


@makbigc did you try @jreback's suggesting of factorizing the EAs before getting into the main if block?

pandas/tests/extension/base/reshaping.py

TomAugspurger · 2018-12-16T20:22:02Z

I'm not too sure about the azure failure, but notice that the data generated includes a duplicate in the first three values

data = [I, a, a, ...

That data is randomly generated. It looks like your expected may not handle cases where the key is not unique.

I believe the only requirement on data are that it's

length 100
the first two values are distinct
the first two values are not NA.

TomAugspurger · 2018-12-18T12:40:02Z

@makbigc I'm going to push a few changes here if that's OK.

…3020

* generic release note * removed _values_for_factorize changes * made tests depend on just the first two * do the factorization first

TomAugspurger · 2018-12-18T13:02:34Z

I started to write a test that would include NA values in the merge key, but I think the expected behavior of that is up in the air right now: #22491.

pandas/core/reshape/merge.py

jreback · 2018-12-18T14:13:43Z

pandas/tests/reshape/merge/test_merge.py

+        # GH 23020
+        df = pd.DataFrame({'A': pd.Series([1, 2, np.nan], dtype='Int64'),
+                           'B': 1})
+        result = pd.merge(df, df, on='A')


I think can remove this?

I guess this does test merging on an NA key, which the generic test doesn't. So maybe leave it?

TomAugspurger · 2018-12-18T14:39:56Z

Updated the coercion of lk / rk to do

all the pre-processing on the values (datetimetz, categorical, extension)
Hashtable / dtype validate

in two distinct blocks.

makbigc · 2018-12-19T14:19:04Z

@TomAugspurger Thanks for your reply and help.

jreback · 2018-12-19T14:29:13Z

thanks @makbigc and @TomAugspurger

maybe we should create some asv's for EA's generally

jreback requested changes Oct 26, 2018

View reviewed changes

jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode ExtensionArray Extending pandas with custom dtypes or arrays. labels Oct 26, 2018

makbigc force-pushed the bug-fix-23020 branch from 406dd00 to 54dd18f Compare November 4, 2018 12:34

jreback requested changes Nov 4, 2018

View reviewed changes

makbigc closed this Nov 8, 2018

makbigc reopened this Nov 8, 2018

makbigc force-pushed the bug-fix-23020 branch 2 times, most recently from 0585c30 to efa30c7 Compare November 17, 2018 14:36

jreback requested changes Nov 17, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.rst Outdated Show resolved Hide resolved

pandas/core/reshape/merge.py Outdated Show resolved Hide resolved

pandas/tests/extension/base/reshaping.py Outdated Show resolved Hide resolved

makbigc force-pushed the bug-fix-23020 branch from b3006e2 to d3efb23 Compare November 18, 2018 14:22

makbigc force-pushed the bug-fix-23020 branch from d3efb23 to b8ed3e9 Compare December 5, 2018 15:19

jreback requested changes Dec 9, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.rst Outdated Show resolved Hide resolved

pandas/core/reshape/merge.py Outdated Show resolved Hide resolved

pandas/tests/extension/base/reshaping.py Outdated Show resolved Hide resolved

makbigc added 5 commits December 16, 2018 22:47

fix bug #GH23020

1e0539d

change after review

9310361

2nd change after review

cb2d4d1

3rd change after review

42477c3

Separate the test and add more cases

6a9e360

makbigc added 7 commits December 16, 2018 22:47

add more types into the test_merge_on_int_array

373582e

isort pandas/core/reshape/merge.py

f817cad

Add _values_for_factorize in period.py

8140ee1

Remove is_period_dtype check

c425b24

Modify _values_for_factorize in sparse.py

29e820d

Test merge with EA as key and open a special case

19621e6

Add factorize method in period.py

a69f7b7

makbigc force-pushed the bug-fix-23020 branch from 6ae8f20 to a69f7b7 Compare December 16, 2018 15:20

TomAugspurger reviewed Dec 16, 2018

View reviewed changes

pandas/core/arrays/period.py Outdated Show resolved Hide resolved

TomAugspurger reviewed Dec 16, 2018

View reviewed changes

TomAugspurger added 2 commits December 18, 2018 06:40

Merge remote-tracking branch 'upstream/master' into makbigc-bug-fix-2…

529f88d

…3020

Updates

61f5e15

* generic release note * removed _values_for_factorize changes * made tests depend on just the first two * do the factorization first

jreback reviewed Dec 18, 2018

View reviewed changes

pandas/core/reshape/merge.py Outdated Show resolved Hide resolved

jreback reviewed Dec 18, 2018

View reviewed changes

Reorganize the if / elifs

6bae6ca

TomAugspurger added this to the 0.24.0 milestone Dec 18, 2018

jreback approved these changes Dec 19, 2018

View reviewed changes

jreback merged commit 14c33b0 into pandas-dev:master Dec 19, 2018

TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request Dec 20, 2018

BUG: merging an Integer EA rasises (pandas-dev#23262)

a88be00

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: merging an Integer EA rasises (pandas-dev#23262)

c986f0b

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: merging an Integer EA rasises (pandas-dev#23262)

08ee17c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: merging an Integer EA rasises #23262

BUG: merging an Integer EA rasises #23262

makbigc commented Oct 21, 2018

pep8speaks commented Oct 21, 2018

jreback Oct 26, 2018

jreback Oct 26, 2018

jreback Oct 26, 2018

TomAugspurger Dec 16, 2018

jreback commented Nov 3, 2018

makbigc commented Nov 4, 2018

jreback Nov 4, 2018

jreback Nov 4, 2018

jreback Nov 4, 2018

TomAugspurger commented Nov 7, 2018

TomAugspurger commented Nov 7, 2018

makbigc commented Nov 8, 2018

TomAugspurger commented Nov 8, 2018 via email

jreback commented Dec 3, 2018

codecov bot commented Dec 5, 2018

codecov bot commented Dec 5, 2018 •

edited

Loading

makbigc commented Dec 6, 2018

TomAugspurger commented Dec 6, 2018

makbigc commented Dec 16, 2018

TomAugspurger Dec 16, 2018

TomAugspurger commented Dec 16, 2018

TomAugspurger commented Dec 18, 2018

TomAugspurger commented Dec 18, 2018

jreback Dec 18, 2018

TomAugspurger Dec 18, 2018

TomAugspurger commented Dec 18, 2018

makbigc commented Dec 19, 2018

jreback commented Dec 19, 2018

BUG: merging an Integer EA rasises #23262

BUG: merging an Integer EA rasises #23262

Conversation

makbigc commented Oct 21, 2018

pep8speaks commented Oct 21, 2018

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 Nov 3, 2018

makbigc commented Nov 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Nov 7, 2018

TomAugspurger commented Nov 7, 2018

makbigc commented Nov 8, 2018

TomAugspurger commented Nov 8, 2018 via email

jreback commented Dec 3, 2018

codecov bot commented Dec 5, 2018

Codecov Report

codecov bot commented Dec 5, 2018 • edited Loading

Codecov Report

makbigc commented Dec 6, 2018

TomAugspurger commented Dec 6, 2018

makbigc commented Dec 16, 2018

Choose a reason for hiding this comment

TomAugspurger commented Dec 16, 2018

TomAugspurger commented Dec 18, 2018

TomAugspurger commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Dec 18, 2018

makbigc commented Dec 19, 2018

jreback commented Dec 19, 2018

codecov bot commented Dec 5, 2018 •

edited

Loading