API: remove ordered requirement in Categorical.searchsorted #21686

topper-123 · 2018-06-29T17:25:34Z

closes PERF: make Categorical.searchsorted not require ordered=True #21667
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

searchsorted is mainly a tool to find value locations in a sorted array. As such it doesn't make sense to restrict it to not work with unordered categoricals. For example, doing searchsorted on ordered, but unsorted arrays gives wrong result, so orderedness is a relatively irrelevant thing to check for in searchsorted.

The current solution to first convert to a ordered Categorical is inelegant and requires setting up different code paths depending on the situation:

if categorical.ordered:
    val = c.searchsorted(key)
else:
    val = c.as_ordered().searchsorted(key)

I've also added to the doc string of searchsorted to make clear that sortedness is required for a correct result (this is not an issue that is limited to categoricals, but all Arrays)

codecov · 2018-06-29T19:32:36Z

Codecov Report

❗ No coverage uploaded for pull request base (master@bbfd4ba). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #21686   +/-   ##
=========================================
  Coverage          ?   91.77%           
=========================================
  Files             ?      174           
  Lines             ?    50704           
  Branches          ?        0           
=========================================
  Hits              ?    46535           
  Misses            ?     4169           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.37% <100%> (?)`
#single	`41.81% <0%> (?)`

Impacted Files	Coverage Δ
pandas/core/base.py	`98.19% <ø> (ø)`
pandas/core/arrays/categorical.py	`95.91% <100%> (ø)`

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 bbfd4ba...a21dc4d. Read the comment docs.

jbrockmendel · 2018-08-17T19:36:37Z

@topper-123 needs rebase

jbrockmendel · 2018-08-17T19:37:04Z

doc/source/whatsnew/v0.24.0.txt

@@ -220,7 +220,8 @@ Bug Fixes
 Categorical
 ^^^^^^^^^^^

-
+- :meth:`Categorical.searchsorted` and :meth:`CategoricalIndex.searchsorted`


I think the standard practice is to keep this as one long line

jbrockmendel · 2018-08-17T19:37:26Z

pandas/core/base.py

@@ -1163,6 +1163,16 @@ def factorize(self, sort=False, na_sentinel=-1):
        corresponding elements in `value` were inserted before the indices,
        the order of `self` would be preserved.

+        .. note::
+
+            The %(klass)s *must* be monotonically sorted, else wrong


“Else wrong” is awkward wording

pandas/core/base.py

jbrockmendel · 2018-08-17T19:39:04Z

pandas/tests/categorical/test_analytics.py

-                         categories=['cheese', 'milk', 'apple', 'bread'],
-                         ordered=False)
-        s2 = Series(c2)
+        c = Categorical(['cheese', 'milk', 'apple', 'bread', 'bread'],


Mind calling these cat and ser or something like that. 1-letter names make grepping tough

jreback · 2018-08-19T12:14:12Z

this needs evaluation
I don’t think we want to allow this
you can easily get incorrect results

topper-123 · 2018-08-19T20:37:50Z

I discuss some of my points in favor of this in #21667:

In searchsorted its already not possible to guard against incorrect result. Also letting this into the code base (.searchsorted on .codes) would just mimic existing sort_values behaviour (sort on .code):

>>> c = pd.Categorical(['a', 'a', 'b', 'b', 'c'], categories=pd.Index(['b', 'a', 'c']))
>>> c.sort_values()
[b, b, a, a, c]
Categories (3, object): [b, a, c]

We can perhaps continue the discussion in #21667.

jorisvandenbossche · 2018-08-20T06:48:29Z

I agree with @topper-123: it is indeed easy to get wrong results with searchsorted, but that is a general issue with that function, and not necessarily specific to Categorical.searchsorted.
searchsorted requires the values to be sorted, but ordered=True is not necessary to sort a Categorical, so IMO should also not be needed for searchsorted

jreback · 2018-08-20T10:30:44Z

.sort_values() is a red-herring here, we are simply lexographically sorting which is expected. Its too easy to make mistakes with .searchsorted here. A doc warning is not great. I would leave this like it is.

jorisvandenbossche · 2018-08-20T11:23:44Z

.sort_values() is a red-herring here, we are simply lexographically sorting which is expected.

Yes, and this sorting will mean that the codes are sorted, so I don't understand why the above is a reason to not do this?

jreback · 2018-08-22T12:58:31Z

Yes, and this sorting will mean that the codes are sorted, so I don't understand why the above is a reason to not do this?

I think its pretty unituitive to sort by codes in an unordered categorical, we should actually disallow .sort_values() as well (or maybe allow it but sort by the lexographic categories themselves).

e.g. we should raise just like we do for min/max.

In [1]: c = pd.Categorical(list('AABCD'))

In [2]: c.min()
TypeError: Categorical is not ordered for operation min
you can use .as_ordered() to change the Categorical to an ordered one


In [3]: c.sort_values()
Out[3]: 
[A, A, B, C, D]
Categories (4, object): [A, B, C, D]

jorisvandenbossche · 2018-08-22T13:02:28Z

Being able to sort is very useful, we discussed this before, so I am -1 on removing that ability (but if you want further discuss this, I would open a new issue).

Note that, in the majority of the use cases for unordered categoricals, sorting by the codes or sorting lexicographically will be identical, exactly because we do sort the categories lexicographically on creation.
So only when passing the categories manually and not setting ordered=True, this might be different.

topper-123 · 2018-08-22T14:42:54Z

I agree with @jorisvandenbossche.

IMO It is beneficial to be able find locations of groups of values unordered values, so lhs, rhs = c.searchssorted(val1, side='left'), c.searchsorted(val1, side='right') should be possible, also for unordered categoricals, because this the fastest way to look up locations (much faster than c[c==val1].

To do the above, unordered categoricals need to have sort_values, is_monotonic* and searchsorted...

mroeschke · 2019-01-06T03:10:52Z

Looks like this stalled. In the meantime, this could use a rebase..

WillAyd · 2019-02-27T23:31:13Z

Closing as stale. Ping if you'd like to continue

topper-123 · 2019-02-28T10:58:51Z

I would actually very much like to continue this, but was under the impression it wasn't going to get in. If you'll open it, I'll push a rebase.

jreback · 2019-02-28T11:23:03Z

will have another look

jreback

can you explain why you think its a good idea to enable this. This looks like it is just cause for trouble.

jreback · 2019-03-03T01:52:26Z

doc/source/whatsnew/v0.24.0.txt

@@ -0,0 +1,731 @@
+.. _whatsnew_0240:
+


this file is included somehow, pls revert

jreback · 2019-03-03T01:52:51Z

doc/source/whatsnew/v0.25.0.rst

@@ -127,7 +127,7 @@ Bug Fixes
 Categorical
 ^^^^^^^^^^^

-
+- :meth:`Categorical.searchsorted` and :meth:`CategoricalIndex.searchsorted` now work on unordered categoricals also (:issue:`21667`)


also -> as well

jreback · 2019-03-03T01:54:34Z

pandas/core/arrays/categorical.py

-        codes = codes[0] if is_scalar(value) else codes
-
-        return self.codes.searchsorted(codes, side=side, sorter=sorter)
+        return algorithms.searchsorted(self._codes, value_codes,


we don't have a guarantee that these codes are sorted right? so this seems likely to return an incorrect result if that is not the case

I've added tests for this test_searchsorted_unordered.

jreback · 2019-03-03T01:55:06Z

pandas/tests/arrays/categorical/test_analytics.py

        exp = np.array([3, 5], dtype=np.intp)
        tm.assert_numpy_array_equal(res_cat, exp)
        tm.assert_numpy_array_equal(res_ser, exp)

        # Searching for a single value that is not from the Categorical
-        pytest.raises(KeyError, lambda: c1.searchsorted('cucumber'))
-        pytest.raises(KeyError, lambda: s1.searchsorted('cucumber'))
+        with pytest.raises(KeyError):


add match= (if that is appropriate)

jreback · 2019-03-03T01:55:27Z

pandas/tests/arrays/categorical/test_analytics.py

-                         categories=['cheese', 'milk', 'apple', 'bread'],
-                         ordered=False)
-        s2 = Series(c2)
+        cat = Categorical(['cheese', 'milk', 'apple', 'bread', 'bread'],


what about a test of the failing case, meaning an unsorted categorical

jreback · 2019-03-26T00:08:28Z

if you can merge master and remove the 0.24.0 docs. pls ping.

jreback · 2019-06-08T20:17:09Z

@topper-123 can you merge master and we'll have another look see

topper-123 · 2019-06-09T10:09:44Z

I've rebased.

jbrockmendel · 2019-08-28T17:02:15Z

@topper-123 can you rebase

WillAyd · 2019-09-13T01:43:03Z

@topper-123 still active?

topper-123 · 2019-09-16T21:13:50Z

Yeah, I've rebased. The PR is quite old, so fixed some strange git issues by just merging the commits into one.

WillAyd · 2019-09-20T14:51:53Z

Taking another look at this again. Read through history but I think I am also on the side of not doing this. Understood on the point about consistency with other aspects of the API but I personally find this confusing, and a doc note isn't a great guard against that.

So I think @jreback and I are -1 here, @topper-123 and @jorisvandenbossche are +1. Still accurate?

This is oldest PR in queue so would like to decide how to move forward so it isn't in limbo forever

jorisvandenbossche · 2019-09-20T17:38:17Z

@WillAyd can you explain what exactly you find confusing?
Note that the doc note is added to the generic searchsorted docstring, as this point is true for all dtypes (you always need to ensure yourself that the values are sorted when calling this method). There is nothing categorical specific about this gotcha with searchsorted.

The only thing with Categoricals is that it can be a bit harder to know if your values are sorted or not. But, you can sort an unordered categorical with sort_values, you can check if they sorted with is_monotonic.
And for searchsorted to work, the only requirement is sortedness, which an unordered categorical still has.

topper-123 · 2019-09-20T21:48:46Z

Agree with everything @jorisvandenbossche said.

I would like to add that searchsorted is used to get fast lookups, so for performance sensitive work, having searchsorted is necessary.

WillAyd · 2019-09-20T23:40:01Z

I find what is quoted in the docstring to be confusing:

If the values are not monotonically sorted, wrong locations
   may be returned:

>>> x = pd.Series([2, 1, 3])
>>> x.searchsorted(1)
0  # wrong result, correct would be 1

I understand that this affects non-Categorical right now, but IMO this should be raising for non-monotonic instead of returning surprising results. I don't think expanding that coverage to non-sorted Categorical moves us in the right direction

jorisvandenbossche · 2019-09-23T10:50:54Z

I understand that this affects non-Categorical right now, but IMO this should be raising for non-monotonic instead of returning surprising results

Then maybe we could split those two issues? Let's open a separate issue to discuss raising for non-monotonic in general, and move forward with this PR making searchsorted available consistently across dtypes?

jorisvandenbossche · 2019-10-01T12:26:22Z

Any more thoughts here?

jreback · 2019-10-01T13:02:59Z

I guess this is fine, though IMHO this is scope creep for ordered categoricals. by-definition unordered ones are not ordered (well they have an order, but it is not exposed to the users per se). that being said, I guess for perf reasons this is desireable.

topper-123 · 2019-10-01T21:27:46Z

I've rebased.

WillAyd · 2019-10-02T03:16:05Z

Thanks @topper-123 for the PR and @jorisvandenbossche for input

…das-dev#21686)

topper-123 force-pushed the Categorical.searchsorted branch 2 times, most recently from 05aabc2 to 97040ca Compare June 29, 2018 19:32

jreback added Categorical Categorical Data Type API Design labels Jul 2, 2018

jbrockmendel reviewed Aug 17, 2018

View reviewed changes

pandas/core/base.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Aug 17, 2018

View reviewed changes

topper-123 force-pushed the Categorical.searchsorted branch from 97040ca to e553898 Compare August 18, 2018 23:17

WillAyd closed this Feb 27, 2019

jreback reopened this Feb 28, 2019

topper-123 force-pushed the Categorical.searchsorted branch from e553898 to b71e252 Compare March 2, 2019 09:24

topper-123 changed the title ~~PERF/API: remove ordered requirement in Categorical.searchsorted~~ API: remove ordered requirement in Categorical.searchsorted Mar 2, 2019

topper-123 force-pushed the Categorical.searchsorted branch from b71e252 to 21e0056 Compare March 2, 2019 22:14

jreback requested changes Mar 3, 2019

View reviewed changes

topper-123 force-pushed the Categorical.searchsorted branch 2 times, most recently from c0fce18 to 9aca098 Compare April 1, 2019 17:39

topper-123 force-pushed the Categorical.searchsorted branch 2 times, most recently from e4931c3 to 9ab6286 Compare June 8, 2019 21:53

topper-123 force-pushed the Categorical.searchsorted branch 4 times, most recently from b143301 to c7ebf9f Compare September 16, 2019 21:11

PERF/API: Remove ordered requirement in Categorical.searchsorted

a21dc4d

topper-123 force-pushed the Categorical.searchsorted branch from c7ebf9f to a21dc4d Compare October 1, 2019 18:42

WillAyd added this to the 1.0 milestone Oct 2, 2019

WillAyd merged commit df10e3f into pandas-dev:master Oct 2, 2019

topper-123 deleted the Categorical.searchsorted branch October 2, 2019 21:21

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

PERF/API: Remove ordered requirement in Categorical.searchsorted (pan…

b687bd8

…das-dev#21686)

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

PERF/API: Remove ordered requirement in Categorical.searchsorted (pan…

f215050

…das-dev#21686)

bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020

PERF/API: Remove ordered requirement in Categorical.searchsorted (pan…

c83df97

…das-dev#21686)

API: remove ordered requirement in Categorical.searchsorted #21686

API: remove ordered requirement in Categorical.searchsorted #21686

Conversation

topper-123 commented Jun 29, 2018 • edited Loading

codecov bot commented Jun 29, 2018 • edited Loading

Codecov Report

jbrockmendel commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Aug 19, 2018

topper-123 commented Aug 19, 2018

jorisvandenbossche commented Aug 20, 2018

jreback commented Aug 20, 2018

jorisvandenbossche commented Aug 20, 2018 • edited Loading

jreback commented Aug 22, 2018

jorisvandenbossche commented Aug 22, 2018

topper-123 commented Aug 22, 2018

mroeschke commented Jan 6, 2019

WillAyd commented Feb 27, 2019

topper-123 commented Feb 28, 2019

jreback commented Feb 28, 2019

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

jreback commented Mar 26, 2019

jreback commented Jun 8, 2019

topper-123 commented Jun 9, 2019

jbrockmendel commented Aug 28, 2019

WillAyd commented Sep 13, 2019

topper-123 commented Sep 16, 2019

WillAyd commented Sep 20, 2019

jorisvandenbossche commented Sep 20, 2019

topper-123 commented Sep 20, 2019

WillAyd commented Sep 20, 2019

jorisvandenbossche commented Sep 23, 2019

jorisvandenbossche commented Oct 1, 2019

jreback commented Oct 1, 2019

topper-123 commented Oct 1, 2019

WillAyd commented Oct 2, 2019

topper-123 commented Jun 29, 2018 •

edited

Loading

codecov bot commented Jun 29, 2018 •

edited

Loading

jorisvandenbossche commented Aug 20, 2018 •

edited

Loading