Skip to content

[PERF] Get rid of MultiIndex conversion in IntervalIndex.is_unique #26391

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 28, 2019
Merged

[PERF] Get rid of MultiIndex conversion in IntervalIndex.is_unique #26391

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4ec1fe9
New IntervalIndex.is_unique
makbigc May 14, 2019
51d6910
Add benchmark
makbigc May 14, 2019
d11acd6
Add whatsnew note
makbigc May 14, 2019
202b2cf
Change after 1st review
makbigc May 15, 2019
8e8384b
Remove relundant code
makbigc May 15, 2019
8dde393
Use set instead of defaultdict
makbigc May 16, 2019
d3af9c9
Lengthen the array in benchmark
makbigc May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions asv_bench/benchmarks/index_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,5 +194,8 @@ def setup(self, N):
def time_monotonic_inc(self, N):
self.intv.is_monotonic_increasing

def time_is_unique(self, N):
self.intv.is_unique


from .pandas_vb_common import setup # noqa: F401
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.25.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ Performance Improvements
- Improved performance of :meth:`Series.map` for dictionary mappers on categorical series by mapping the categories instead of mapping all values (:issue:`23785`)
- Improved performance of :meth:`read_csv` by faster concatenating date columns without extra conversion to string for integer/float zero
and float NaN; by faster checking the string for the possibility of being a date (:issue:`25754`)
- Improved performance of :meth:`IntervalIndex.is_unique` by removing conversion to `MultiIndex` (:issue:`24813`)

.. _whatsnew_0250.bug_fixes:

Expand Down
23 changes: 22 additions & 1 deletion pandas/core/indexes/interval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,28 @@ def is_unique(self):
"""
Return True if the IntervalIndex contains unique elements, else False
"""
return self._multiindex.is_unique
left = self.values.left
right = self.values.right
Copy link
Member

Choose a reason for hiding this comment

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

self.values.left should be equivalent to self.left, so I think we can get by without needing to define these, and just refer to them as self.left/self.right where needed


def _is_unique(left, right):
Copy link
Member

Choose a reason for hiding this comment

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

If my previous comment is correct, I don't think we need this to be a function anymore since it's only called once, so you can just put the function's logic at the end of the method.

Can you also test out the following variant of _is_unique:

from collections import defaultdict


def _is_unique2(left, right):
    seen_pairs = defaultdict(bool)
    check_idx = np.where(left.duplicated(keep=False))[0]
    for idx in check_idx:
        pair = (left[idx], right[idx])
        if seen_pairs[pair]:
            return False
        seen_pairs[pair] = True

    return True

I did a sample run of this, and it appears to be a bit more efficient:

In [3]: np.random.seed(123)
   ...: left = pd.Index(np.random.randint(5, size=10**5))
   ...: right = pd.Index(np.random.randint(10**5/4, size=10**5))

In [4]: %timeit _is_unique(left, right)
3.84 ms ± 34.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [5]: %timeit _is_unique2(left, right)
1.13 ms ± 26.3 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

I haven't fully tested this in all scenarios though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HEAD adopts _is_unique2 and HEAD~3 adopts _is_unique. The performance is slightly worse but the code is more explanatory.

       before           after         ratio
     [4ec1fe97]       [202b2cfa]
     <intv-is-unique~3>       <intv-is-unique>
        230±0.2ns          217±2ns     0.94  index_object.IntervalIndexMethod.time_is_unique(1000)
+         277±2ns          316±5ns     1.14  index_object.IntervalIndexMethod.time_is_unique(100000)

Copy link
Contributor

Choose a reason for hiding this comment

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

are these asv's really short? maybe have a longer one and see how this scales

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was wondering this too; is_unique is cached, so I wonder if the asv is just timing the cache lookup? Does anything special need to be done to handle things that are cached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HEAD~6 adopts _is_unique while HEAD adopts _is_unique2.

       before           after         ratio
     [4ec1fe97]       [d3af9c91]
     <intv-is-unique~6>       <intv-is-unique>
          223±1ns          207±1ns     0.93  index_object.IntervalIndexMethod.time_is_unique(1000)
+         270±5ns          302±4ns     1.12  index_object.IntervalIndexMethod.time_is_unique(100000)
       1.50±0.01s          1.84±0s    ~1.22  index_object.IntervalIndexMethod.time_is_unique(10000000)

# left must have at least one common point
duplicates = left[left.duplicated()].unique()
for dup in duplicates:
# Check whether the Intervals having the same left endpoint
# also have the same right endpoint
if not right[left == dup].is_unique:
return False
return True

if len(self) - len(self.dropna()) > 1:
Copy link
Member

Choose a reason for hiding this comment

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

I think self.isna().sum() > 1 is a little more idiomatic and performant.

Doing single runs to avoid caching:

In [2]: ii = pd.interval_range(0, 10**5)

In [3]: ii_nan = ii.insert(1, np.nan).insert(12345, np.nan)

In [4]: %timeit -r 1 -n 1 ii.isna().sum() > 1
435 µs ± 0 ns per loop (mean ± std. dev. of 1 run, 1 loop each)

In [5]: %timeit -r 1 -n 1 ii_nan.isna().sum() > 1
444 µs ± 0 ns per loop (mean ± std. dev. of 1 run, 1 loop each)

In [6]: ii = pd.interval_range(0, 10**5)

In [7]: ii_nan = ii.insert(1, np.nan).insert(12345, np.nan)

In [8]: %timeit -r 1 -n 1 len(ii) - len(ii.dropna()) > 1
677 µs ± 0 ns per loop (mean ± std. dev. of 1 run, 1 loop each)

In [9]: %timeit -r 1 -n 1 len(ii_nan) - len(ii_nan.dropna()) > 1
2.18 ms ± 0 ns per loop (mean ± std. dev. of 1 run, 1 loop each)

return False

if left.is_unique and right.is_unique:
return True
elif not left.is_unique:
return _is_unique(left, right)
else:
return _is_unique(right, left)

@cache_readonly
@Appender(_interval_shared_docs['is_non_overlapping_monotonic']
Expand Down