API: ExtensionArray.argsort places the missing value at the end #27137

makbigc · 2019-06-30T05:21:11Z

continue #26354

Categorical will be handled in a separate PR.
benchmark result:

       before           after         ratio
     [989f912e]       [dc0ef701]
     <master>         <enh-21801-2>
+     11.4±0.03μs       36.8±0.1μs     3.23  algorithms.EASorting.time_argsort(1000)
+       785±0.5μs      1.26±0.03ms     1.60  algorithms.EASorting.time_argsort(100000)

xref #21801 (doesn't close; Need to update existing EAs)

TomAugspurger

@makbigc can you add a test in the base extension array tests?

diff --git a/pandas/tests/extension/base/methods.py b/pandas/tests/extension/base/methods.py
index d9e61e6a2..17315a52e 100644
--- a/pandas/tests/extension/base/methods.py
+++ b/pandas/tests/extension/base/methods.py
@@ -52,6 +52,11 @@ class BaseMethodsTests(BaseExtensionTests):
         expected = pd.Series(np.array([1, -1, 0], dtype=np.int64))
         self.assert_series_equal(result, expected)
 
+    def test_argsort_missing_array(self, data_missing_for_sorting):
+        result = np.argsort(data_missing_for_sorting)
+        expected = data_missing_for_sorting.take([2, 0, 1])
+        self.assert_extension_array_equal(result, expected)
+
     @pytest.mark.parametrize('na_position, expected', [
         ('last', np.array([2, 0, 1], dtype=np.dtype('intp'))),
         ('first', np.array([1, 2, 0], dtype=np.dtype('intp')))

If you don't have time today, LMK. I may push some changes to get this in.

asv_bench/benchmarks/algorithms.py

doc/source/whatsnew/v0.25.0.rst

pandas/tests/extension/test_categorical.py

* rework test * xfail categorical * rename asv * update whatsnew

pandas/core/arrays/categorical.py

TomAugspurger · 2019-07-03T13:47:23Z

FYI @jorisvandenbossche just doing the breaking Categorical change here.

TomAugspurger · 2019-07-03T14:56:49Z

As expected, a few failures in our test suite. Going through them next.

I think https://travis-ci.org/pandas-dev/pandas/jobs/553767675#L2498 is buggy / defined strangely. (DataFrame.sort_index with a CategoricalIndex and ascending=False)

# 0.24
In [2]: df = pd.DataFrame({"A": list(range(6))}, index=pd.Categorical(['a', 'a', 'b', 'b', 'c', 'a'], categories=['c', 'a', 'b']))

In [3]: df.sort_index(ascending=False)
Out[3]:
   A
b  3
b  2
a  5
a  1
a  0
c  4

In [4]: df.set_index(df.index.codes).sort_index(ascending=False)
Out[4]:
   A
2  2
2  3
1  0
1  1
1  5
0  4

# this PR

In [3]: df.sort_index()
Out[3]:
   A
c  4
a  0
a  1
a  5
b  2
b  3

In [4]: df.set_index(df.index.codes).sort_index()
Out[4]:
   A
0  4
1  0
1  1
1  5
2  2
2  3

I think that sorting by a CategoricalIndex should be the same as sorting by its codes. 0.24 (and master, before the PR) doesn't have that property.

jorisvandenbossche · 2019-07-03T14:58:54Z

It's probably more a comment on the changes in #26854, but it seems that nargsort is now always calling _values_for_argsort for EAs. But, in the comments of ExtensionArray.argsort, we say that there are two ways to override argsort: have _values_for_argsort, or implement argsort yourself for full control.

So the current indirection seems a bit wrong. At least depending on where nargsort is called internally, it should call the argsort method of the EA, to let the EA have full control over it.

(but that is maybe not blocking for the RC, most important is to get the actual behaviour change in)

TomAugspurger · 2019-07-03T15:38:43Z

Hmm, so we would want to tell users that they should always override _values_for_argsort, since just doing .argsort may not be enough. OK, I'll try to write something about that.

jorisvandenbossche · 2019-07-03T18:28:13Z

Hmm, so we would want to tell users that they should always override _values_for_argsort, since just doing .argsort may not be enough.

Not fully sure. We could also pass _values_for_argsort to nargsort from withing EA.argsort, and let nargsort call EA.argsort (I think, might be missing something in this indirection web).

TomAugspurger · 2019-07-03T18:35:51Z

I'm also having trouble with the indirection.

AFAICT, with this PR we end up in a situation where ExtensionArray.argsort could differ from other places in pandas that do nargsort(ExtensionArray). Since nargsort Does pandas' usual sorting algo on the ExtensionArray._values_for_argsort...

TomAugspurger · 2019-07-03T20:21:08Z

All green. I had to slightly adjust the test in 47c6b57

I don't have a 32-bit machine to test on, but it seems like we may be inconsistent with whether _values_for_argsort returns an int32 or int64 dtype array. It doesn't seem to matter in the algorithm, so I cast to int64 before making the assert.

jreback · 2019-07-03T20:38:56Z

All green. I had to slightly adjust the test in 47c6b57

I don't have a 32-bit machine to test on, but it seems like we may be inconsistent with whether _values_for_argsort returns an int32 or int64 dtype array. It doesn't seem to matter in the algorithm, so I cast to int64 before making the assert.

I believe this will break on the 32-bit builds; argsort is supposed to return platform ints.

So I guess our current interface is kind wrong. But let's merge this and consider that.

jreback · 2019-07-03T20:39:55Z

pandas/core/arrays/base.py

@@ -409,7 +410,8 @@ def argsort(self, ascending=True, kind='quicksort', *args, **kwargs):
        Returns
        -------
        index_array : ndarray


so we aren't specific that these are actually intp (and NOT int64's)

jorisvandenbossche · 2019-07-03T20:48:43Z

AFAICT, with this PR we end up in a situation where ExtensionArray.argsort could differ from other places in pandas that do nargsort(ExtensionArray). Since nargsort Does pandas' usual sorting on the ExtensionArray._values_for_argsort...

Let's open an issue about that to fix after the RC before final 0.25.0 ?

TomAugspurger · 2019-07-03T20:50:00Z

Writing one now.

…

On Wed, Jul 3, 2019 at 3:48 PM Joris Van den Bossche < ***@***.***> wrote: AFAICT, with this PR we end up in a situation where ExtensionArray.argsort could differ from other places in pandas that do nargsort(ExtensionArray). Since nargsort Does pandas' usual sorting on the ExtensionArray._values_for_argsort... Let's open an issue about that to fix after the RC before final 0.25.0 ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27137?email_source=notifications&email_token=AAKAOIW4K5LZLPQF6565JFLP5UGDHA5CNFSM4H4LZM7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFUMEQ#issuecomment-508249618>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIQWYYAC45IW5HCW3W3P5UGDHANCNFSM4H4LZM7A> .

TomAugspurger · 2019-07-03T20:57:38Z

#27218, but my thinking on this may be a bit unclear. On Wed, Jul 3, 2019 at 3:49 PM Tom Augspurger <[email protected]> wrote:

…

Writing one now. On Wed, Jul 3, 2019 at 3:48 PM Joris Van den Bossche < ***@***.***> wrote: > AFAICT, with this PR we end up in a situation where > ExtensionArray.argsort could differ from other places in pandas that do > nargsort(ExtensionArray). Since nargsort Does pandas' usual sorting on the > ExtensionArray._values_for_argsort... > > Let's open an issue about that to fix after the RC before final 0.25.0 ? > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#27137?email_source=notifications&email_token=AAKAOIW4K5LZLPQF6565JFLP5UGDHA5CNFSM4H4LZM7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFUMEQ#issuecomment-508249618>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAKAOIQWYYAC45IW5HCW3W3P5UGDHANCNFSM4H4LZM7A> > . >

makbigc added 2 commits June 30, 2019 13:28

Use nargsort inside EA.argsort to sort

0b3d22d

Add benchmark

79df1d8

makbigc force-pushed the enh-21801-2 branch from c2809f3 to 79df1d8 Compare June 30, 2019 05:29

makbigc mentioned this pull request Jun 30, 2019

API: ExtensionArray.argsort places the missing value at the end #26354

Closed

jorisvandenbossche added this to the 0.25.0 milestone Jun 30, 2019

jreback removed this from the 0.25.0 milestone Jul 2, 2019

jreback added ExtensionArray Extending pandas with custom dtypes or arrays. Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Jul 2, 2019

TomAugspurger reviewed Jul 2, 2019

View reviewed changes

asv_bench/benchmarks/algorithms.py Outdated Show resolved Hide resolved

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

pandas/tests/extension/test_categorical.py Outdated Show resolved Hide resolved

jorisvandenbossche mentioned this pull request Jul 2, 2019

API: argsort behaviour for ExtensionArray with missing values #21801

Closed

TomAugspurger added 2 commits July 3, 2019 08:28

Merge remote-tracking branch 'upstream/master' into makbigc-enh-21801-2

01e3686

Updates

a5d46ce

* rework test * xfail categorical * rename asv * update whatsnew

jreback reviewed Jul 3, 2019

View reviewed changes

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

TomAugspurger added 2 commits July 3, 2019 08:44

API: break Categorical.argsort

b1b96a7

issue

c5bfb44

TomAugspurger added 4 commits July 3, 2019 11:47

update for break

c9868c1

update for break

7bedc4b

Merge remote-tracking branch 'upstream/master' into makbigc-enh-21801-2

6b1871c

trigger CI

c917575

TomAugspurger added this to the 0.25.0 milestone Jul 3, 2019

TomAugspurger added 2 commits July 3, 2019 13:50

32-bit compat

bd2cbfd

32-bit compat

47c6b57

jreback reviewed Jul 3, 2019

View reviewed changes

jreback merged commit 47ffcd6 into pandas-dev:master Jul 3, 2019

jorisvandenbossche mentioned this pull request Apr 4, 2020

EA interface - requirements for "hashable, value+order-preserving ndarray" #33276

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: ExtensionArray.argsort places the missing value at the end #27137

API: ExtensionArray.argsort places the missing value at the end #27137

makbigc commented Jun 30, 2019 •

edited by TomAugspurger

Loading

TomAugspurger left a comment

TomAugspurger commented Jul 3, 2019

TomAugspurger commented Jul 3, 2019

jorisvandenbossche commented Jul 3, 2019 •

edited

Loading

TomAugspurger commented Jul 3, 2019

jorisvandenbossche commented Jul 3, 2019

TomAugspurger commented Jul 3, 2019 •

edited

Loading

TomAugspurger commented Jul 3, 2019

jreback commented Jul 3, 2019

jreback Jul 3, 2019

jorisvandenbossche commented Jul 3, 2019

TomAugspurger commented Jul 3, 2019 via email

TomAugspurger commented Jul 3, 2019 via email

API: ExtensionArray.argsort places the missing value at the end #27137

API: ExtensionArray.argsort places the missing value at the end #27137

Conversation

makbigc commented Jun 30, 2019 • edited by TomAugspurger Loading

TomAugspurger left a comment

Choose a reason for hiding this comment

TomAugspurger commented Jul 3, 2019

TomAugspurger commented Jul 3, 2019

jorisvandenbossche commented Jul 3, 2019 • edited Loading

TomAugspurger commented Jul 3, 2019

jorisvandenbossche commented Jul 3, 2019

TomAugspurger commented Jul 3, 2019 • edited Loading

TomAugspurger commented Jul 3, 2019

jreback commented Jul 3, 2019

jreback Jul 3, 2019

Choose a reason for hiding this comment

jorisvandenbossche commented Jul 3, 2019

TomAugspurger commented Jul 3, 2019 via email

TomAugspurger commented Jul 3, 2019 via email

makbigc commented Jun 30, 2019 •

edited by TomAugspurger

Loading

jorisvandenbossche commented Jul 3, 2019 •

edited

Loading

TomAugspurger commented Jul 3, 2019 •

edited

Loading