ENH - Index set operation modifications to address issue #23525 #23538

aa1371 · 2018-11-07T04:05:23Z

This is a first pass at addressing the associated issue. Needs plenty of discussion, feedback and testing.

closes Why is pd.Index.union not commutative? #23525
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

…empty indexes, and allow more cross index operaions

pep8speaks · 2018-11-07T04:05:29Z

Hello @ArtinSarraf! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-21 00:53:39 UTC

jreback · 2018-11-07T05:19:43Z

i would write tests before writing any code

…coerce mismatched closings on interval indexes

…ng cast to dtype object

…airs

aa1371 · 2018-11-11T06:29:45Z

@jreback / @TomAugspurger / @gfyoung - Added tests for combinations of mismatched types. As well as for compatible inconsistent pairs (i.e. Range/Int64Index). Existing tests have been updated to account for the change in behavior and all pytest pandas/tests pass.

Any feedback on whether the changes are on the right track?

jreback

@ArtinSarraf on good track.

pandas/core/indexes/category.py

pandas/core/indexes/base.py

jreback · 2018-11-11T16:05:00Z

pandas/core/indexes/base.py


+        # if is_dtype_equal(self.dtype, other.dtype):


can you put some comments here on the decisions here?

All subclass implementations started with this check, so thought it would make sense to pull this out to be common among all, and implement index specific overriden behavior in the _union. I do override the union methods in subclasses to account for docstring changes though. Was planning on coming up with a better way to override the docstring without having to override and make a call to super. Is there any good way to do this already used within pandas?

pandas/core/indexes/interval.py

pandas/core/indexes/period.py

pandas/core/indexes/range.py

jreback · 2018-11-11T16:08:47Z

pandas/tests/indexes/test_setops.py

+    return pd.Index([])
+
+
+INDEXES = dict(


we already have fixtures in pandas/tests/indexes/conftest.py pls use them instead of creating new ones like this. you may need to create derived fixtures which is ok.

will look into it, thanks

The existing fixture used pre-instantiated indexes. Added a fixture for the index factories.

pandas/tests/indexes/test_setops.py

pandas/tests/reshape/test_concat.py

jreback · 2018-11-11T16:11:49Z

when you push always rebase again master.

aa1371 · 2018-11-13T03:24:30Z

@jreback - regarding moving setop tests into the base level:
Right now all the setop tests are specific to an Index type and are segregated into their own respective index subdirectory. Combining them all together would create a 1000+ line test module with mixed test styles which might be harder to parse through than the separate files. Should we leave as is, or would one module with clear section headers be sufficient?

pandas/core/indexes/interval.py

pandas/tests/indexes/interval/test_interval.py

pandas/tests/indexes/period/test_setops.py

jreback

@ArtinSarraf looks really good. some small comments. pls merge master and ping on green.

jreback · 2019-03-20T01:16:16Z

pandas/tests/indexes/datetimes/test_setops.py

        cases = [klass(second.values) for klass in [np.array, Series, list]]
        for case in cases:
+            print('hi')


remove the print!

jreback · 2019-03-20T01:16:29Z

pandas/tests/indexes/datetimes/test_setops.py

@@ -30,10 +30,14 @@ def test_union2(self, sort):
        tm.assert_index_equal(union, everything)

        # GH 10149
+        expected = first.astype('O').union(
+            pd.Index(second.values, dtype='O')
+        ).astype('O')
        cases = [klass(second.values) for klass in [np.array, Series, list]]


ideally can parametrize this here

jreback · 2019-03-20T01:16:56Z

pandas/tests/indexes/period/test_setops.py

@@ -120,10 +120,6 @@ def test_union_misc(self, sort):
        with pytest.raises(period.IncompatibleFrequency):
            index.union(index2, sort=sort)

-        msg = 'can only call with other PeriodIndex-ed objects'


do we have cases where we would raise ever?

I do not believe so. Only the incompatible frequency error which is covered in the line above.

pandas/tests/indexes/test_setops.py

jreback · 2019-03-20T01:18:12Z

pandas/tests/indexes/test_setops.py

+                         COMPATIBLE_INCONSISTENT_PAIRS.values())
+def test_compatible_inconsistent_pairs(idx_fact1, idx_fact2):
+    # GH 23525
+    idx1 = idx_fact1(10)


were you able to remove any existing tests that are duplicative here? (maybe in test_base? or indexes/test_setops?)

Considering the only "compatible inconsistent pair" currently is RangeIndex/Int64Index it seems that indexes/test_range is much more thorough then this test I have here. It would make more sense to remove the test here.

However, I find this test to be convenient since it is very clear what pairs make up special cases, and adds very little overhead.

TomAugspurger · 2019-03-21T12:57:38Z

What was the resolution of the different, but sometimes compatible dtypes like uint and int? What does np.concatenate do here?

…

On Mar 20, 2019, at 20:59, ArtinSarraf ***@***.***> wrote: @ArtinSarraf commented on this pull request. In pandas/tests/indexes/test_setops.py: > + # This is true before this PR as well. + + # Union with a non-unique, non-monotonic index raises error + # This applies to the boolean index + idx1 = idx1.sort_values() + idx2 = idx2.sort_values() + + assert idx1.union(idx2).dtype == np.dtype('O') + assert idx2.union(idx1).dtype == np.dtype('O') + + ***@***.***('idx_fact1,idx_fact2', + COMPATIBLE_INCONSISTENT_PAIRS.values()) +def test_compatible_inconsistent_pairs(idx_fact1, idx_fact2): + # GH 23525 + idx1 = idx_fact1(10) Considering the only "compatible inconsistent pair" currently is RangeIndex/Int64Index it seems that indexes/test_range is much more thorough then this test I have here. It would make more sense to remove the test here. However, I find this test to be convenient since it is very clear what pairs make up special cases, and adds very little overhead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jschendel · 2019-03-21T23:15:31Z

What does np.concatenate do here?

Looks like it casts to float64 in every scenario:

In [2]: a1 = np.array([1], dtype='int64')

In [3]: a2 = np.array([2], dtype='uint64')

In [4]: np.concatenate([a1, a2]).dtype
Out[4]: dtype('float64')

In [5]: a3 = np.array([-1], dtype='int64')

In [6]: a4 = np.array([np.iinfo(np.uint64).max], dtype='uint64')

In [7]: np.concatenate([a3, a4]).dtype
Out[7]: dtype('float64')

aa1371 · 2019-03-27T04:40:04Z

I feel like this supports the current behavior of single consistent type. However, I guess it also raises the possibility of casting non-consistent numeric types to float64.

Thoughts?

TomAugspurger · 2019-03-27T15:30:28Z

We typically use object as the “default” type, where numpy uses float. I think for these cases like uint and int, we’ll just want object for the dtype stability. May need a depreciation cycle.

…

On Mar 26, 2019, at 23:40, ArtinSarraf ***@***.***> wrote: I feel like this supports the current behavior of single consistent type. However, I guess it also raises the possibility of casting non-consistent numeric types to float64. Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aa1371 · 2019-03-28T22:56:34Z

@TomAugspurger - what does “depreciation cycle” mean in this context. Also do you think there should be any action related to that in this PR?

TomAugspurger · 2019-03-28T23:24:13Z

Avoiding the behavior change for those dtypes. May need to require users to astype ahead of time. I haven’t looked at this PR recently.

…

On Mar 28, 2019, at 17:56, ArtinSarraf ***@***.***> wrote: @TomAugspurger - what does “depreciation cycle” mean in this context. Also do you think there should be any action related to that in this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jreback

just a couple of very minor comments (except the last one), ping on green.

jreback · 2019-05-15T23:49:07Z

pandas/tests/indexes/datetimes/test_setops.py

@@ -29,11 +29,20 @@ def test_union2(self, sort):
        union = first.union(second, sort=sort)
        tm.assert_index_equal(union, everything)

+    @pytest.mark.parametrize("klass", [np.array, Series, list])


minor nit, we usually call this box rather than klass

jreback · 2019-05-15T23:50:11Z

pandas/tests/indexes/test_setops.py

+@pytest.fixture(params=list(it.combinations(indices_list, 2)),
+                ids=lambda x: type(x[0]).__name__ + type(x[1]).__name__)
+def index_pair(request):
+    '''


can you use triple-double quotes

jreback · 2019-05-15T23:51:44Z

pandas/tests/series/test_operators.py

+                                             raises=InvalidIndexError,
+                                             strict=True)),
+        ops.rxor,
+    ])


is this orthogonal to this PR?

This is due to the discussion here:
#23538 (comment)

ok fair enough.

jreback · 2019-05-16T00:23:42Z

lgtm. if any comments @TomAugspurger

aa1371 · 2019-05-17T02:08:04Z

@jreback - tests passed.
@TomAugspurger / @jschendel any other comments?

jreback · 2019-05-19T19:00:18Z

lgtm. @ArtinSarraf can you merge master and ping on green

aa1371 · 2019-05-21T01:57:27Z

@jreback - merged and green

TomAugspurger · 2019-05-21T20:41:32Z

Alrighty, let's merge this. Thanks for sticking with it @ArtinSarraf!

…3525 (pandas-dev#23538)

ENH - first pass at modifying set operations on indexes. Dont ignore …

c2cf269

…empty indexes, and allow more cross index operaions

gfyoung added Dtype Conversions Unexpected or buggy dtype conversions API Design Period Period data type Indexing Related to indexing on series/frames, not to indexes themselves labels Nov 7, 2018

gfyoung requested review from jreback and TomAugspurger November 7, 2018 04:09

Artin Sarraf added 9 commits November 7, 2018 19:39

Merge branch 'master' into enh/index_setops

435e50f

BUG - account for empty index + non-monotonic index, and dont try to …

4922fd3

…coerce mismatched closings on interval indexes

TST - update existing tests to account for cross type index joins bei…

5e528a1

…ng cast to dtype object

ENH - incompatibility checks and incompatible type unions

cdaa5b0

TST - update datetime union tets, add tests for inconsistent unions

40d57ec

CLN - refactor union -> _union

11fd041

TST - add tests for categrorical index, and compatible inconsistent p…

8f0ace3

…airs

BUG - union -> _union in overriden _union methods

8364c2e

TST - update test_operator raised exception

ab329a9

jreback requested changes Nov 11, 2018

View reviewed changes

Artin Sarraf added 3 commits November 11, 2018 12:23

CLN - pep8 line adherence

93486ad

ENH - reverse polarity of compatibility check and add docstrings

e435e4c

Merge branch 'master' into enh/index_setops

b9787b8

TST - add test fixture for index factories and use in test_setops

2241b65

jschendel reviewed Nov 13, 2018

View reviewed changes

pandas/core/indexes/interval.py Outdated Show resolved Hide resolved

jschendel reviewed Nov 13, 2018

View reviewed changes

pandas/tests/indexes/interval/test_interval.py Outdated Show resolved Hide resolved

jschendel reviewed Nov 13, 2018

View reviewed changes

pandas/tests/indexes/interval/test_interval.py Outdated Show resolved Hide resolved

jschendel reviewed Nov 13, 2018

View reviewed changes

pandas/tests/indexes/period/test_setops.py Outdated Show resolved Hide resolved

jreback requested changes Mar 20, 2019

View reviewed changes

Artin Sarraf added 3 commits March 19, 2019 21:52

Merge branch 'master' into enh/index_setops

fbf3242

TST - parametrize union tests

38d9f74

Merge branch 'master' into enh/index_setops

b9e7b18

DOC - add docstring

69aaa93

Merge branch 'master' into enh/index_setops

b57160a

Artin Sarraf added 3 commits May 14, 2019 23:00

Merge branch 'master' into enh/index_setops

daa1287

CLN/TST - fix super method calls and add error msg

54898c1

TST - add Timestamp to regexand fix import sorting

fa839a9

jreback requested changes May 15, 2019

View reviewed changes

CLN - minor style updates

a36f475

jreback approved these changes May 16, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone May 16, 2019

Merge branch 'master' into enh/index_setops

b840f49

TomAugspurger merged commit 20d0ad1 into pandas-dev:master May 21, 2019

another-green pushed a commit to another-green/pandas that referenced this pull request May 29, 2019

ENH - Index set operation modifications to address issue pandas-dev#2…

9af03ce

…3525 (pandas-dev#23538)

jorisvandenbossche mentioned this pull request Jun 11, 2019

REGR: Index.union preserve numerical index for range/float and int/float combinations #26778

Closed

aa1371 mentioned this pull request Aug 22, 2019

REGR: Index.union should pick correct dtype for combinations of tstamps with different tzones #28034

Closed

codeape2 mentioned this pull request Oct 31, 2019

datetime64[ns] dtype coerced to object after pd.concat(axis=1) #28786

Closed

ENH - Index set operation modifications to address issue #23525 #23538

ENH - Index set operation modifications to address issue #23525 #23538

Conversation

aa1371 commented Nov 7, 2018

pep8speaks commented Nov 7, 2018 • edited Loading

Comment last updated at 2019-05-21 00:53:39 UTC

jreback commented Nov 7, 2018

aa1371 commented Nov 11, 2018 • edited Loading

jreback 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

jreback commented Nov 11, 2018

aa1371 commented Nov 13, 2018

jreback 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

TomAugspurger commented Mar 21, 2019 via email

jschendel commented Mar 21, 2019

aa1371 commented Mar 27, 2019

TomAugspurger commented Mar 27, 2019 via email

aa1371 commented Mar 28, 2019

TomAugspurger commented Mar 28, 2019 via email

jreback 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

jreback commented May 16, 2019

aa1371 commented May 17, 2019

jreback commented May 19, 2019

aa1371 commented May 21, 2019

TomAugspurger commented May 21, 2019

pep8speaks commented Nov 7, 2018 •

edited

Loading

aa1371 commented Nov 11, 2018 •

edited

Loading