PERF: RangeIndex.is_monotonic_inc/dec by sinhrks · Pull Request #13749 · pandas-dev/pandas (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation7 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

sinhrks

as the same as is_unique, it should be monotonic by definition

%timeit -n 1 -r 1 pd.RangeIndex(0, 1000000).is_monotonic_increasing

# current master
#1 loop, best of 1: 17.2 ms per loop

# this PR
#1 loop, best of 1: 45.1 µs per loop

@codecov-io

jorisvandenbossche

self.assertTrue(index.is_monotonic)
self.assertTrue(index.is_monotonic_increasing)
self.assertTrue(index.is_monotonic_decreasing)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a test for the empty case as well?

In [23]: pd.RangeIndex().is_monotonic_increasing
Out[23]: True

In [24]: pd.RangeIndex().is_monotonic_decreasing
Out[24]: True

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this should maybe rather raise an error (@jreback)

Other index types to raise without arguments, and range() does as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisvandenbossche empty case is coverd by (1, 1).

pd.RangeIndex(1, 1).values
# array([], dtype=int64)

Let me issue a separate PR to prohibit empty construction.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, assumed that RangeIndex() does not has a step yet, but that is of course wrong :-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I don't think RangeIndex() should be permitted, though I seem to remember needing it for unpickling or somesuch.......

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make a new issue about the empty RangeIndex()

@sinhrks