-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: Implement MultiIndex.is_monotonic_decreasing #17455
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lgtm. @TomAugspurger can you give a once over. |
This looks fine, though I wonder if we can mirror the implementation of @property
def is_monotonic_decreasing(self):
"""
return if the index is monotonic decreasing (only equal or
decreasing) values.
"""
values = [self._get_level_values(i).values
for i in reversed(range(len(self.levels)))]
try:
sort_order = np.lexsort(values)
return Index(sort_order).is_monotonic_decreasing
except TypeError:
# we have mixed types and np.lexsort is not happy
return Index(self.values).is_monotonic_decreasing @jschendel does that pass the tests you wrote, or am I missing something? |
maybe make a helper function for this? (and just call in both) e.g.
|
Yeah, it's borderline too much duplication to implement them both like that. I could go either way, but would probably have a helper. |
@TomAugspurger : That doesn't quite work if the MultiIndex has dupes, e.g. The workaround I came up with was to add a fake unique decreasing level to I did some ad hoc timings to decide which implementation to use, and the overhead of reversing the index was actually less than the overhead of adding an additional level to sort by in For reference, here's my implementation with the fake decreasing level to force uniqueness. Perhaps there are some optimizations I missed. def is_monotonic_decreasing(self):
"""
return if the index is monotonic decreasing (only equal or
decreasing) values.
"""
# initialize values as decreasing range to force uniqueness, needed
# since lexsort() only sorts ascending and want dupes in decreasing order
values = [np.arange(len(self) - 1, -1, -1)]
# reversed() because lexsort() wants the most significant key last.
values += [self._get_level_values(i).values
for i in reversed(range(len(self.levels)))]
try:
sort_order = np.lexsort(values)
return Index(sort_order).is_monotonic_decreasing
except TypeError:
# we have mixed types and np.lexsort is not happy
return Index(self.values).is_monotonic_decreasing I guess we could potentially use some type of heuristic based on size to choose between which method to use (similar to how |
@jschendel can you post an asv of the is_monotonic for various index types (maybe against 0.20.3) |
@jreback : I haven't used asv before but it doesn't look too hard to use, so I should be able to get some benchmarks. I'm a little confused as to what you're looking for? I'm not changing the Are you looking for an asv of |
I think we already have some asv's for is_monotonic and friends, just want to be sure we have coverage for MultiIndex. |
@jschendel I think this was fine, can you rebase |
3c4b8cc
to
a6e4f69
Compare
@jreback : rebased Regarding the asv's for |
that looks like the place; I think we should have asv for these properties. it can do later if u want this PR.lgtm ping on green |
Implemented MultiIndex.is_monotonic_decreasing, and added associated tests. Also added tests for IntervalIndex.is_monotonic_decreasing, as it uses MultiIndex under the hood.
a6e4f69
to
ae6320b
Compare
Codecov Report
@@ Coverage Diff @@
## master #17455 +/- ##
==========================================
- Coverage 91.22% 91.2% -0.02%
==========================================
Files 163 163
Lines 49625 49625
==========================================
- Hits 45270 45261 -9
- Misses 4355 4364 +9
Continue to review full report at Codecov.
|
ping @jreback : green |
Thanks @jschendel! |
Implemented MultiIndex.is_monotonic_decreasing, and added associated tests. Also added tests for IntervalIndex.is_monotonic_decreasing, as it uses MultiIndex under the hood.
Implemented MultiIndex.is_monotonic_decreasing, and added associated tests. Also added tests for IntervalIndex.is_monotonic_decreasing, as it uses MultiIndex under the hood.
git diff upstream/master -u -- "*.py" | flake8 --diff
Note that this is a dupe PR of #16573, which appears to have gone stale.
Implementation ended up being easier than what I suggested in the issue; can use the fact that an index is monotonic decreasing if it's reverse is monotonic increasing. Just a matter of reversing the index and calling
is_monotonic_increasing
.Regarding
MultiIndex
tests:is_monotonic_increasing
test to verify it's working for an empty index.is_monotonic_decreasing
test cases consist of decreasing versions of the test cases in theis_monotonic_increasing
test.Regarding
IntervalIndex
tests:IntervalIndex.is_monotonic_increasing
andIntervalIndex.is_monotonic_decreasing
since they use the MultiIndex implementation under the hood.is_unique
into separate tests. Expanded the test cases for both tests.