Skip to content
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

API: change datetimelike Index to raise IndexError instead ValueError #18386

Merged

Conversation

Projects
None yet
2 participants
@jorisvandenbossche
Copy link
Member

commented Nov 20, 2017

I noticed this in the plotting PR that DatetimeIndex et al behave a bit strange on an invalid index:

In [41]: pd.Index([1, 2, 3])['no_int']
...
IndexError: only integers, slices (`:`), ellipsis (`...`), numpy.newaxis (`None`) and integer or boolean arrays are valid indices

In [42]: pd.DatetimeIndex([1, 2, 3])['no_int']
...
ValueError: 

I think this should be consistent IndexError ?

if indices.size != 100:
return

exp = getattr(indices, '_box_func', lambda x: x)(indices._values[0])

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Nov 20, 2017

Author Member

This is a bit too much 'internal-details-relying' for a test, but I didn't directly know a way to write it generic for all types of index instead (or maybe we just shouldn't try, and keep it only testing the errors)

This comment has been minimized.

Copy link
@jreback

jreback Nov 21, 2017

Contributor

you don't need this at all, just check for the error

@jorisvandenbossche jorisvandenbossche added this to the 0.22.0 milestone Nov 20, 2017

@codecov

This comment has been minimized.

Copy link

commented Nov 20, 2017

Codecov Report

Merging #18386 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18386      +/-   ##
==========================================
- Coverage   91.36%   91.34%   -0.02%     
==========================================
  Files         164      164              
  Lines       49718    49718              
==========================================
- Hits        45426    45417       -9     
- Misses       4292     4301       +9
Flag Coverage Δ
#multiple 89.14% <100%> (ø) ⬆️
#single 39.61% <0%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/datetimelike.py 97.11% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.8% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1647a72...3bf0378. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Nov 20, 2017

Codecov Report

Merging #18386 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18386      +/-   ##
==========================================
+ Coverage   91.32%   91.41%   +0.08%     
==========================================
  Files         164      157       -7     
  Lines       49802    51378    +1576     
==========================================
+ Hits        45484    46965    +1481     
- Misses       4318     4413      +95
Flag Coverage Δ
#multiple 89.27% <100%> (+0.16%) ⬆️
#single 40.57% <0%> (-0.31%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/datetimelike.py 96.91% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/util/testing.py 81.8% <0%> (-18.2%) ⬇️
pandas/io/json/json.py 91.75% <0%> (-8.25%) ⬇️
pandas/io/formats/style.py 96.21% <0%> (-3.79%) ⬇️
pandas/core/frame.py 97.81% <0%> (-0.1%) ⬇️
pandas/core/indexes/datetimes.py 95.61% <0%> (-0.1%) ⬇️
pandas/tests/io/json/__init__.py
pandas/tests/util/__init__.py
pandas/tests/io/msgpack/__init__.py
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a47ad56...27931f6. Read the comment docs.


assert exp == arr[5]
if indices.size != 100:

This comment has been minimized.

Copy link
@jreback

jreback Nov 21, 2017

Contributor

why do you have this?

if indices.size != 100:
return

exp = getattr(indices, '_box_func', lambda x: x)(indices._values[0])

This comment has been minimized.

Copy link
@jreback

jreback Nov 21, 2017

Contributor

you don't need this at all, just check for the error

@@ -591,12 +591,19 @@ def test_empty_fancy(self):
# Index.
pytest.raises(IndexError, idx.__getitem__, empty_farr)

def test_getitem(self):

This comment has been minimized.

Copy link
@jreback

jreback Nov 21, 2017

Contributor

this original test is pretty useless, so no need to keep its function (and this is tested else in many places as well)
all of the indexes have a test_indexing.py

This comment has been minimized.

Copy link
@jreback

jreback Nov 21, 2017

Contributor

just rename the test to test_getitem_error. if you want you can split this out into a new file, test_indexing.py in this dir.

@jreback jreback added the API Design label Nov 21, 2017

@jreback
Copy link
Contributor

left a comment

add a note in whatsnew indexing bug fixes (0.22)

@jreback

jreback approved these changes Dec 1, 2017

@jreback jreback merged commit 1eedcf6 into pandas-dev:master Dec 1, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.