Added RuntimeWarning to lib.fast_unique_multiple #33015

ShaharNaveh · 2020-03-25T14:25:39Z

pandas/_libs/lib.pyx

jbrockmendel · 2020-03-25T17:22:20Z

pandas/tests/test_lib.py

@@ -39,6 +40,11 @@ def test_fast_unique_multiple_list_gen_sort(self):
        out = lib.fast_unique_multiple_list_gen(gen, sort=False)
        tm.assert_numpy_array_equal(np.array(out), expected)

+    def test_fast_unique_multiple_unsortable_runtimewarning(self):
+        idx = pd.MultiIndex.from_product([[1, pd.Timestamp("2000")], ["foo", "bar"]])


can you target this test more specifically at the affected function? maybe pass np.array([Timestamp(...), "foo"])?

It's not a rule/policy, but ideally the intra-pandas dependencies of test_lib would be limited to the dependencies of lib.

ShaharNaveh · 2020-03-25T19:27:33Z

Looks like at L340, it triggers the warning, but only at the AP Linux py37_np_dev

pandas/pandas/tests/indexes/multi/test_setops.py

Lines 334 to 345 in 42ef409

    
           def test_union_sort_other_incomparable(): 
        
               # https://github.com/pandas-dev/pandas/issues/24959 
        
               idx = pd.MultiIndex.from_product([[1, pd.Timestamp("2000")], ["a", "b"]]) 
        
               # default, sort=None 
        
               result = idx.union(idx[:1]) 
        
               tm.assert_index_equal(result, idx) 
        
               # sort=False 
        
               result = idx.union(idx[:1], sort=False) 
        
               tm.assert_index_equal(result, idx)

@jbrockmendel Does having something like this:

from pandas.compat.numpy import _is_numpy_dev

...

if _is_numpy_dev:
    with tm.assert_produces_warning(RuntimeWarning):
        result = idx.union(idx[:1]) 
else:
    result = idx.union(idx[:1])

is the correct approach?

jbrockmendel · 2020-03-25T21:03:06Z

Usually we would write

warn = RuntimeWarning if _is_numpy_dev else None
with tm.assert_produces_warning(warn):
    ...

WillAyd · 2020-03-26T00:13:10Z

pandas/tests/test_lib.py

+    def test_fast_unique_multiple_unsortable_runtimewarning(self):
+        arr = [np.array(["foo", pd.Timestamp("2000")])]
+        with tm.assert_produces_warning(RuntimeWarning):
+            lib.fast_unique_multiple(arr, sort=None)


Can you test this from an end user perspective?

pandas/tests/indexes/multi/test_setops.py

…ng-_libs-lib

jorisvandenbossche · 2020-03-26T09:01:10Z

pandas/_libs/lib.pyx

@@ -286,7 +287,7 @@ def fast_unique_multiple(list arrays, sort: bool = True):
        try:
            uniques.sort()
        except TypeError:
-            # TODO: RuntimeWarning?
+            warnings.warn("The values in the array are unorderable", RuntimeWarning)


Can you indicate somethig about what to do about this? (eg passing the sort=False keyword? I don't know if that is possible for all cases that may end up here)

…ng-_libs-lib

XREF: pandas-dev#33015 (comment)

pandas/_libs/lib.pyx

Co-Authored-By: Joris Van den Bossche <[email protected]>

Everything else failed

…ng-_libs-lib

jbrockmendel · 2020-03-27T21:06:12Z

pandas/tests/indexes/multi/test_setops.py

@@ -332,12 +334,15 @@ def test_union_sort_other_empty_sort(slice_):
    tm.assert_index_equal(result, expected)


+@pytest.mark.xfail(reason="`warn` if statement is not working properly", strict=False)


why is this necesssary? non-strict xfails are among the patterns we want to avoid

This is necessary because I couldn't get the CI to pass otherwise

OK, lets troubleshoot that rather than xfail.

Why is there no expected warning on numpy-dev?

I don't know, I have tried in d2cd2d9 to have:

warn = None if _is_numpy_dev else RuntimeWarning

We can trouble shoot from there.

…ng-_libs-lib

ShaharNaveh · 2020-03-29T12:14:47Z

So apparently I never needed the _is_numpy_dev. *facepalm*

jreback

lgtm. can you add a release note, can put in Other Enhancements for 1.1

…ng-_libs-lib

jreback

ping on green.

jreback · 2020-03-30T22:34:34Z

doc/source/whatsnew/v1.1.0.rst

@@ -87,6 +87,7 @@ Other enhancements
 - Positional slicing on a :class:`IntervalIndex` now supports slices with ``step > 1`` (:issue:`31658`)
 - :class:`Series.str` now has a `fullmatch` method that matches a regular expression against the entire string in each row of the series, similar to `re.fullmatch` (:issue:`32806`).
 - :meth:`DataFrame.sample` will now also allow array-like and BitGenerator objects to be passed to ``random_state`` as seeds (:issue:`32503`)
+- :meth:`MultiIndex.union` will now raise `RuntimeWarning` if the object inside are unsortable, pass `sort=False` to suppress this warning.


can you add the issue number at the end (this PR number is fine if we don't have an issue)

XREF: pandas-dev#33015 (comment)

…ng-_libs-lib

ShaharNaveh · 2020-03-31T07:09:12Z

ping @jreback

…ng-_libs-lib

jreback · 2020-04-02T14:25:09Z

thanks @MomIsBestFriend

Added RuntimeWarning to lib.fast_unique_multiple

a8cbc37

ShaharNaveh commented Mar 25, 2020

View reviewed changes

pandas/_libs/lib.pyx Outdated Show resolved Hide resolved

isort

28871d1

jbrockmendel reviewed Mar 25, 2020

View reviewed changes

MomIsBestFriend added 2 commits March 25, 2020 20:36

Fixed test case to be lib specific

3db5519

Warning error message

8a7f6c9

Not asserting the warning if on numpy_dev

9ce23ab

WillAyd requested changes Mar 26, 2020

View reviewed changes

WillAyd added the Performance Memory or execution speed performance label Mar 26, 2020

ShaharNaveh commented Mar 26, 2020

View reviewed changes

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

MomIsBestFriend added 3 commits March 26, 2020 03:03

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

d57aded

…ng-_libs-lib

Troubleshoot failing test

1c3a45f

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

9db8893

…ng-_libs-lib

jorisvandenbossche reviewed Mar 26, 2020

View reviewed changes

MomIsBestFriend added 3 commits March 26, 2020 17:13

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

ab8cc80

…ng-_libs-lib

Added information on how to suppress the warning

d186e6d

XREF: pandas-dev#33015 (comment)

Troubleshoot failing test

df301eb

ShaharNaveh requested review from WillAyd and jorisvandenbossche March 26, 2020 15:45

jorisvandenbossche reviewed Mar 26, 2020

View reviewed changes

pandas/_libs/lib.pyx Outdated Show resolved Hide resolved

ShaharNaveh and others added 4 commits March 26, 2020 21:10

Update pandas/_libs/lib.pyx

fde1e1a

Co-Authored-By: Joris Van den Bossche <[email protected]>

Marked failed test as xfail

6d1508d

Everything else failed

Marked xfailed-strict as False

1314e99

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

f29760c

…ng-_libs-lib

jbrockmendel reviewed Mar 27, 2020

View reviewed changes

MomIsBestFriend added 3 commits March 29, 2020 12:12

Troubleshoot failing test

e55bc81

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

d2cd2d9

…ng-_libs-lib

Removed if statement related to numpy-dev

b931025

jreback added API - Consistency Internal Consistency of API/Behavior Error Reporting Incorrect or improved errors from pandas and removed Performance Memory or execution speed performance labels Mar 29, 2020

jreback added this to the 1.1 milestone Mar 29, 2020

jreback requested changes Mar 29, 2020

View reviewed changes

MomIsBestFriend added 3 commits March 29, 2020 19:03

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

2c05ed2

…ng-_libs-lib

Added whatsnew

17a6f99

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

b7ace2b

…ng-_libs-lib

jreback requested changes Mar 30, 2020

View reviewed changes

MomIsBestFriend added 2 commits March 31, 2020 09:38

What's new refrence to this PR

83e2e82

XREF: pandas-dev#33015 (comment)

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

564d416

…ng-_libs-lib

Merge remote-tracking branch 'upstream/master' into TODO-runtimewarni…

63a6104

…ng-_libs-lib

jreback approved these changes Apr 2, 2020

View reviewed changes

jreback merged commit 92478d5 into pandas-dev:master Apr 2, 2020

ShaharNaveh deleted the TODO-runtimewarning-_libs-lib branch April 2, 2020 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added RuntimeWarning to lib.fast_unique_multiple #33015

Added RuntimeWarning to lib.fast_unique_multiple #33015

ShaharNaveh commented Mar 25, 2020

jbrockmendel Mar 25, 2020

ShaharNaveh commented Mar 25, 2020 •

edited

Loading

jbrockmendel commented Mar 25, 2020

WillAyd Mar 26, 2020

jreback Apr 2, 2020

jorisvandenbossche Mar 26, 2020

jbrockmendel Mar 27, 2020

ShaharNaveh Mar 27, 2020

jbrockmendel Mar 27, 2020

ShaharNaveh Mar 29, 2020

ShaharNaveh commented Mar 29, 2020

jreback left a comment

jreback left a comment

jreback Mar 30, 2020

ShaharNaveh commented Mar 31, 2020

jreback commented Apr 2, 2020

		@@ -332,12 +334,15 @@ def test_union_sort_other_empty_sort(slice_):
		tm.assert_index_equal(result, expected)


		@pytest.mark.xfail(reason="`warn` if statement is not working properly", strict=False)

Added RuntimeWarning to lib.fast_unique_multiple #33015

Added RuntimeWarning to lib.fast_unique_multiple #33015

Conversation

ShaharNaveh commented Mar 25, 2020

Choose a reason for hiding this comment

ShaharNaveh commented Mar 25, 2020 • edited Loading

jbrockmendel commented Mar 25, 2020

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

ShaharNaveh commented Mar 29, 2020

jreback left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaharNaveh commented Mar 31, 2020

jreback commented Apr 2, 2020

ShaharNaveh commented Mar 25, 2020 •

edited

Loading