New Interval / IntervalIndex behavior spec
State: closed

 - [x] part of #16316
 - [x] tests added / passed
 - [x] passes ``git diff upstream/master --name-only -- '*.py' | flake8 --diff``

@jreback 

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Interval
description: Interval data type

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16386?src=pr&el=h1) Report
> Merging [#16386](https://codecov.io/gh/pandas-dev/pandas/pull/16386?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/ef753900b5c3956c43068ff64d271e1332024f52?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/16386/graphs/tree.svg?width=650&height=150&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/16386?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #16386      +/-   ##
==========================================
- Coverage   91.57%   91.56%   -0.02%     
==========================================
  Files         150      150              
  Lines       48913    48959      +46     
==========================================
+ Hits        44794    44831      +37     
- Misses       4119     4128       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.93% <ø> (-0.01%)` | :arrow_down: |
| #single | `41.13% <ø> (-0.04%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16386?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `92.08% <0%> (-0.48%)` | :arrow_down: |
| [pandas/tseries/offsets.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvb2Zmc2V0cy5weQ==) | `96.78% <0%> (-0.29%)` | :arrow_down: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `92.77% <0%> (-0.23%)` | :arrow_down: |
| [pandas/core/indexes/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZWxpa2UucHk=) | `97.13% <0%> (-0.07%)` | :arrow_down: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `90.17% <0%> (-0.04%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.06% <0%> (-0.03%)` | :arrow_down: |
| [pandas/core/resample.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzYW1wbGUucHk=) | `96.34% <0%> (-0.02%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.44% <0%> (-0.01%)` | :arrow_down: |
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.07% <0%> (-0.01%)` | :arrow_down: |
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `96.85% <0%> (ø)` | :arrow_up: |
| ... and [7 more](https://codecov.io/gh/pandas-dev/pandas/pull/16386/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16386?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16386?src=pr&el=footer). Last update [ef75390...acefcb0](https://codecov.io/gh/pandas-dev/pandas/pull/16386?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I just took a quick look and this looks great!

One comment: it would be great to convert your tests for Series indexing into unit tests on the IntervalIndex itself, which would let us avoids testing the unrelated indexing code. In most cases, the methods to test are `.get_loc` and `.get_indexer`.
@shoyer sorry, I might need a little more explanation. Where can I find those methods, i.e. where should I move my unit tests to? 
See here for the existing IntervalIndex tests: https://github.com/pandas-dev/pandas/blob/master/pandas/tests/indexes/test_interval.py#L329
@shoyer sorry, still lost -- don't know the internals of pandas too well I guess. 

What exactly do you mean by putting the unit tests on IntervalIndex itself rather than on the indexing code? I think the reason I'm confused is I don't know how `.loc` works -- does it call `.get_loc`? What is the "bottom" method that we should be testing? I don't see `loc` in `pandas/pandas/core/indexes/interval.py` ...
No worries! The "bottom" methods are .get_loc(), .slice_locs() and
.get_indexer():
http://pandas.pydata.org/pandas-docs/stable/internals.html#indexing

On Sun, May 21, 2017 at 10:49 AM, Alexander Lenail <notifications@github.com
> wrote:

> @shoyer <https://github.com/shoyer> sorry, still lost -- don't know the
> internals of pandas too well I guess.
>
> What exactly do you mean by putting the unit tests on IntervalIndex itself
> rather than on the indexing code? I think the reason I'm confused is I
> don't know how .loc works -- does it call .get_loc? What is the "bottom"
> method that we should be testing? I don't see loc in
> pandas/pandas/core/indexes/interval.py ...
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/16386#issuecomment-302941387>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABKS1m8HMYPNxgY6g7-KoWYwYB1FCpGaks5r8E8TgaJpZM4Nftfz>
> .
>

@shoyer 

1. should `get_loc` really return an int if there's a single match and an array if there are multiple matches? 

2. Where is the implementation of `get_loc`? Is it in the `pyx` / `pxi.in`? Where can I learn more about what those are?

3. Does `__getitem__` go through `get_loc`? If not, where should I move the `__getitem__` tests? 

4. In `indexing/test_interval.py`, there's `test_loc_with_scalar` and in `indexes/test_interval.py` there's `test_get_loc_value` which tests the same logic, I think. It might make sense to do a slightly more complete re-shuffling of the tests if we want to make each test unique, and assign a correct file for each test..
@zfrenchee 

> should get_loc really return an int if there's a single match and an array if there are multiple matches?

Yes.

> Where is the implementation of get_loc? Is it in the pyx / pxi.in? Where can I learn more about what those are?

All these methods are defined on `IntervalIndex`: https://github.com/pandas-dev/pandas/blob/49ec31bbaeca81a6f58fc1be26fe80f3ac188cdd/pandas/core/indexes/interval.py

> Does __getitem__ go through get_loc? If not, where should I move the __getitem__ tests?

Yes, almost always. The notable exception is for cases where it performs integer indexing rather than label based indexing.

> In indexing/test_interval.py, there's test_loc_with_scalar and in indexes/test_interval.py there's test_get_loc_value which tests the same logic, I think.

The tests in `indexing/test_interval.py` test indexing on objects with an IntervalIndex. These are useful as integration tests.

The tests in `indexes/test_interval.py` test the IntervalIndex behavior alone, and thus are unit tests.

Both unit tests and integration tests are useful, but only unit tests need to comprehensively check every possible behavior (there are too many ways to combine things for integration tests).
> The tests in indexing/test_interval.py test indexing on objects with an IntervalIndex. These are useful as integration tests.
> 
> The tests in indexes/test_interval.py test the IntervalIndex behavior alone, and thus are unit tests.

@shoyer, @jreback  suggested adding the tests to `indexing/test_interval.py` but all the tests are really unit tests. Should I move *all* my tests in that case? I think that makes sense, just want the okay from you. 
@shoyer still waiting to hear about where these tests belong. Since I basically only added unit tests, I think it might make sense to move *all* the tests to indexes from indexing. Let me know what you think. 
@zfrenchee in your case, yes, I think almost all of your unit tests should be in `indexes`. We already have plenty of integration tests (though a few of those might need fixes).
@shoyer  would you take a look? I added a new `test_get_loc_value` function, and moved all my covers / overlaps tests to indexes for good measure. 

1. I'm happy to trim the overlaps / covers tests down if both those functions will end up calling .get_loc. 

2. I'm happy to add overlaps / covers integration tests (right now there are no more tests for those functions in indexing. 

3. I haven't written any new `slice_locs` tests, mostly because I'm not sure what this is testing on the index / am generally confused about slicing. 

4. there are a couple "updated" integration tests that still need work in indexing. 

Let me know what you think / where we should go from here 
> I haven't written any new slice_locs tests, mostly because I'm not sure what this is testing on the index / am generally confused about slicing.

`slice_locs` is used for label based indexing with slices. Basically, `series.loc[start:stop]` gets implemented as something like `series.iloc[series.index.slice_locs(start, stop)`. (Don't worry about `step`, it's only allowed to be an integer and doesn't really effect how this works.)

To gain some intuition for how this should work, I recommend trying indexing a series with integer labels, e.g., `s = pd.Series(range(5))` with various integer and float bounds. Also try with out of order `index=[3, 4, 0, 1, 2]`, duplicate `index=[0, 0, 1, 1, 2]` or decreasing `index=reversed(range(5))` indexes.

I think `IntervalIndex` handles slicing consistently with `Int64Index`, but I haven't tested it much recently.

Generally, I like the way this is coming together -- these test cases will be super helpful! See inline comments below.
good to know, thanks!
On Thu, May 25, 2017 at 7:52 PM Jeff Reback <notifications@github.com>
wrote:

> *@jreback* commented on this pull request.
> ------------------------------
>
> In pandas/tests/indexes/test_interval.py
> <https://github.com/pandas-dev/pandas/pull/16386#discussion_r118625713>:
>
> > +
> +        assert     Interval(1, 3, closed='right').covers(Interval(1, 3, closed='right'))
> +        assert not Interval(1, 3, closed='right').covers(Interval(1, 3, closed='left'))
> +        assert not Interval(1, 3, closed='right').covers(Interval(1, 3, closed='both'))
> +
> +        assert not Interval(1, 3, closed='left').covers(Interval(1, 3, closed='right'))
> +        assert     Interval(1, 3, closed='left').covers(Interval(1, 3, closed='left'))
> +        assert not Interval(1, 3, closed='left').covers(Interval(1, 3, closed='both'))
> +
> +        assert     Interval(1, 3, closed='both').covers(Interval(1, 3, closed='right'))
> +        assert     Interval(1, 3, closed='both').covers(Interval(1, 3, closed='left'))
> +        assert     Interval(1, 3, closed='both').covers(Interval(1, 3, closed='both'))
> +
> +        idx = IntervalIndex.from_tuples([(0, 1), (2, 3), (1, 3)])
> +
> +        assert     Interval(1, 3, closed='right').covers(idx) == np.array([1, 2])
>
> nope use
>
> tm.assert_numpy_array_equal
>
> we never directly use numpy comparison routines
>
> the linter will fail if it sees np.testing fyi
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/16386#discussion_r118625713>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABKS1khw7Ui5dtvxfyKz4ql1Tg80ezH8ks5r9j5_gaJpZM4Nftfz>
> .
>

@shoyer slice locs is a funky method. It seems special-cased when the index is monotonically increasing or decreasing -- is that right? 

Thanks for your review by the way! I fixed some stuff up, I think it's mostly ready for another look.

I'm particularly interested in hearing: 
- should tests be moved? 
- should tests be split? 
- should some tests be reduced because their functionality is already being tested elsewhere (e.g. overlaps & covers deferring to get_loc)
- Are some tests missing? In particular some of the integration tests in indexing might need new equivalents under the "updated" regime, but I'm not totally sure. 

Finally, I think (but am not totally sure) I need to take a long look at `slice_locs` and write some tests for that. I'll take a look at that later today. 
@shoyer I've taken another quick pass, but I'm not entirely sure what to do for slicing. 

Would you take a quick look and answer the questions I posted above as well? 

> @shoyer slice locs is a funky method. It seems special-cased when the index is monotonically increasing or decreasing -- is that right?

Yes, correct.

> should some tests be reduced because their functionality is already being tested elsewhere (e.g. overlaps & covers deferring to get_loc)

At least in this case, I would still test these separately.

More detailed comments to follow after I look at your changes in detail...
@shoyer @jreback I can think of a couple possible sensible behaviors for `slice locs`:

Going back to @jreback 's example cases:

### `slice locs` is basically an alias for `covers` in the case of ints

```
df = pd.DataFrame(['a'], columns=['col'], 
          index=pd.IntervalIndex.from_tuples([(10, 15)], closed='both'))

df.loc[11:13]      # []
df.loc[9:16]       # a
df.loc[10:15]     # a
```

### `slice locs` requires exact matches in the context of slices between intervals

```
df = pd.DataFrame(['a', 'b', 'c'], columns=['col'], 
          index=pd.IntervalIndex.from_tuples([(11, 13), (14, 15), (16, 17)], closed='both'))

df.loc[pd.Interval(11, 13):pd.Interval(14, 15)]    # a, b
df.loc[pd.Interval(11, 13):pd.Interval(16, 17)]    # a, b, c
df.loc[pd.Interval(11, 12):pd.Interval(14, 15)]    # KeyError
df.loc[pd.Interval(11, 15):pd.Interval(16, 17)]    # KeyError
df.loc[pd.Interval(11, 13):]    # a, b, c
```

Let me know what you guys think, or whether it makes sense to open this topic back up in #16316.
@shoyer @jreback I think this PR is in need of a thorough review. 


@shoyer bump
I have a busy week at work, I probably won't be able to take another look
until next week sometime.

On Wed, Jun 14, 2017 at 7:15 AM, Alexander Lenail <notifications@github.com>
wrote:

> @shoyer <https://github.com/shoyer> bump
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/16386#issuecomment-308444825>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABKS1lK2-os6_m1YLNViaKs32O4Ar-gtks5sD-sJgaJpZM4Nftfz>
> .
>

Pinging myself: will try to look at this tomorrow or friday.
@jorisvandenbossche Thanks for the review!

> It would be great if you could also write some narrative with the indexing rules that follow your new added tests.

What exactly do you mean? Where would I add this narrative? 
>> It would be great if you could also write some narrative with the indexing rules that follow your new added tests.

> Where would I add this narrative?

For now maybe just start discussing it here in the comments, but later something like that should certainly be added to the docs.

> What exactly do you mean? 

Currently the 'rules' are embedded in the tests, but for someone who wants to know how indexing with an IntervalIndex works, having to look at the tests is not the way to go (and even just for discussing the desired semantics here in the PR, it will be helpful as well).

Example of 'rules' is:

- For `loc` indexing:
  - indexing with a scalar (-> `get_loc`):
    - with `Interval` objects: only exact matches (exact match = all of left, right and closed should be equal) (so as opposed to current behaviour in master)
    - with actual values: selects those that contain the value
  - indexing with a slice:
    - with Interval objects: only exact matches (and index has to be sorted I think?)
    - with actual values: should the selected intervals be fully contained in the slice, or should the slice start/end be contained in the Interval? (I already don't remember from the tests)
  - indexing with a list
    - ...
- ...
Okay, so as @shoyer [mentioned above](https://github.com/pandas-dev/pandas/pull/16386#issuecomment-302942312), the bottom methods are  `.get_loc()`, `.slice_locs()` and `.get_indexer()` and so we should at least update the functionality for those three. 

Right now, I think that `.get_loc()` is in a good place, but `.slice_locs()` and `.get_indexer()` are not super well defined. 

### `get_indexer`

What should we do in each of the following situations: 

```
IntervalIndex.get_indexer(int/float)
IntervalIndex.get_indexer(list of ints/floats)
IntervalIndex.get_indexer(Interval)
IntervalIndex.get_indexer(list of Interval)
IntervalIndex.get_indexer(IntervalIndex)
IntervalIndex.get_indexer(list of IntervalIndex)
```

@jorisvandenbossche already mostly answers [above](https://github.com/pandas-dev/pandas/pull/16386#discussion_r122749593) 

### `slice_locs`

[My comment above about `.slice_locs()` hasn't been answered yet. ](https://github.com/pandas-dev/pandas/pull/16386#issuecomment-306800091)

What should we do in each of the following situations: 

```
IntervalIndex.slice_locs(int/float:int/float)
IntervalIndex.slice_locs(int/float:)
IntervalIndex.slice_locs(:int/float)

IntervalIndex.slice_locs(list of ints/floats pairs)

IntervalIndex.slice_locs(Interval:Interval)
IntervalIndex.slice_locs(Interval:)
IntervalIndex.slice_locs(:Interval)

IntervalIndex.slice_locs(list of Interval pairs)

IntervalIndex.slice_locs(IntervalIndex)
IntervalIndex.slice_locs(list of IntervalIndex)
```

Some of those can probably throw "that doesn't make any sense" errors, but I'm not sure what the seemingly valid ones should do. @jorisvandenbossche @jreback 

@jorisvandenbossche, with respect to your adding milestone 0.21.0, I would really love to see this stuff in production as soon as possible, and so I believe we should do more than introduce these xfail tests by 0.21.0 but also implement the functionality they describe. Was that what you were thinking? 
@zfrenchee I may have some time to do this in a few weeks. A few important caveats. Having tests that are self-consistent is the most important thing here. Please mark tests (via comment is fine), on ones where this is the correct way, with no ambiguity and there is one obvious answer.

Any tests which are in any way subjective should be segregated to separate tests, and marked this way.

What happes with an implementation is that you make the most important / most correct things work first. Having tests that we are still discussing makes things very confusing.
@jreback Would you please elaborate on 

> Please mark tests (via comment is fine), on ones where this is the correct way, with no ambiguity and there is one obvious answer.

I'm not sure I understand. 
I mean that each test function should be fairly narrow and focused with an indicative title / comment.

test_interval_boolen_no_overlap or somesuch. these are *much much* easier to debug when the cases are clearly enumerated.