Skip to content

MAINT: More informative TypeError for IndexEngine.get_loc #12414

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

MAINT: More informative TypeError for IndexEngine.get_loc #12414

wants to merge 1 commit into from

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Feb 22, 2016

Title is self-explanatory. Closes #12218.

@gfyoung
Copy link
Member Author

gfyoung commented Feb 22, 2016

Are either of the following necessary for this PR:

  1. What's New (if yes, which section?)
  2. Unit test (if yes, where?)

@jreback
Copy link
Contributor

jreback commented Feb 22, 2016

I don't believe this ever becomes an external exception

@gfyoung
Copy link
Member Author

gfyoung commented Feb 22, 2016

But the issue that I'm closing showed an example where the Exception was thrown

@jreback
Copy link
Contributor

jreback commented Feb 22, 2016

so this is right

so add a test that asserts the contents of the error message

it's s type error because the indexer is not the correct type to select the level

@gfyoung
Copy link
Member Author

gfyoung commented Feb 22, 2016

Added a test, and the test passes on Travis. Should be good to merge if there is nothing else.

@jorisvandenbossche jorisvandenbossche added the Error Reporting Incorrect or improved errors from pandas label Feb 22, 2016
jreback
jreback reviewed Feb 22, 2016
View reviewed changes
pandas/tests/frame/test_indexing.py
@@ -2590,3 +2590,19 @@ def test_head_tail(self):
empty_df = DataFrame()
assert_frame_equal(empty_df.tail(), empty_df)
assert_frame_equal(empty_df.head(), empty_df)

def test_type_error_multiindex(self):
df = pd.DataFrame(columns=['i', 'c', 'x', 'y'],
Copy link
Contributor

Choose a reason for hiding this comment

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

add the issue number here

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@gfyoung
Copy link
Member Author

gfyoung commented Feb 22, 2016

Not really sure why Python 3.4 is failing, as the failing test case has no relation to what I changed. I'm going to rebase onto master so as to trigger Travis again.

jreback
jreback reviewed Feb 22, 2016
View reviewed changes
pandas/tests/frame/test_indexing.py
str(dg[:, 0])

# Neither of the following commands should
# throw a TypeError because they are valid
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to put these asserts around here, just run the actual commands. if they raise the tests will fail; this just obfuscuates tings. best is actually to test the results

e.g.

result = dg.loc[:, (slice(None), 0)]
expected = ....
assert_.....

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@gfyoung gfyoung force-pushed the invalid_key_inform branch 2 times, most recently from 149921b to 0dac883 Compare February 23, 2016 01:27
@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

Tests are passing, so this should be good to merge if there is nothing else.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

can you add a whatsnew note (you can put in bug fixes is fine). ping when pushed.

@jreback jreback added this to the 0.18.0 milestone Feb 23, 2016
@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

018.0 is ok

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

Sorry, as soon as I posted the question, I saw you had already added it to the 0.18.0 milestone, which made the answer somewhat obvious. 😄

jreback
jreback reviewed Feb 23, 2016
View reviewed changes
doc/source/whatsnew/v0.18.0.txt
@@ -1117,3 +1117,4 @@ Bug Fixes
- Bug in ``DataFrame.apply`` in which reduction was not being prevented for cases in which ``dtype`` was not a numpy dtype (:issue:`12244`)
- Bug when initializing categorical series with a scalar value. (:issue:`12336`)
- Bug when specifying a UTC ``DatetimeIndex`` by setting ``utc=True`` in ``.to_datetime`` (:issue:11934)
- Bug in ``IndexEngine.get_loc`` in which an empty ``TypeError`` was being thrown instead of one with an error message (:issue:12218)
Copy link
Contributor

Choose a reason for hiding this comment

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

just say that you added a meaningful error message when multi-axis indexing with an invalid value.

these whatsnotes are for the average user. Those who want/need more detail can simply click on the issue itself. But for a quick glance, just need to highlite terms that most users would know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, okay. Good to know.

@gfyoung gfyoung force-pushed the invalid_key_inform branch 3 times, most recently from 00b9545 to 831c3a1 Compare February 23, 2016 17:08
jreback
jreback reviewed Feb 23, 2016
View reviewed changes
pandas/tests/frame/test_indexing.py
labels=[[0, 1], [0, 0]],
names=[None, 'c'])
values = np.array([[1, 2], [3, 4]], dtype=np.int64)
index = Index(data=[0, 1], dtype='int64', name='i')
Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see you the index has a name, ok then.

Still construct like this

index = Index(range(2), name='i') it automatically is int64
you can just construct

expected = DataFrame([[1, 2], [3, 4], columns=columns, dtype='int64')

Copy link
Contributor

Choose a reason for hiding this comment

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

no need to intermix numpy arrays at all here. Its just confusing to a reader.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing. Done.

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

Could you cancel some of my duplicate, older builds? I see no reason for them to run given all of the updating I have done. Thanks!

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

done, do you know any way to configure that on Travis? (e.g. it builds pull-requests, but I want force pushes to cancel anything but the last one).

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

Unfortunately not. See here.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

actually I think this will work as the ref commit is now gone. (if you force push on the same branch)

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

I'm not entirely sure I follow what you mean since I do force pushes all time with my PR's in pandas and haven't seen that happen.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

what I mean is that if you force push, then when Travis tries to build it is fails (but is grey so doesn't count) and doesn't build the previous commit (but instead builds your new one)

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

On a separate note, is there a pandas-svn mailing list (similar to numpy and scipy) that GitHub can send notifications to to let us know when merges / commits have been made to the master branch? I find such notifications to be quite useful so that PR branches don't get too far behind the master branch.

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

Regarding Travis: that should work I suppose, but I haven't seen that work consistently in practice, at least not in this library. IDK if there's a way to guarantee Travis can't find the old ref.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

@gfyoung where is this list defined?

yes easy to add to Travis

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

Oh, I meant : does such a mailing list exist?

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

@gfyoung could create one, what I meant was does numpy define one somewhere?

@jorisvandenbossche @TomAugspurger would this be useful?

@gfyoung
Copy link
Member Author

gfyoung commented Feb 23, 2016

@jreback : If you visit this link here, you'll see it there. Every time PR's get merged into master or commits are made, everyone on that list gets notified.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2016

@gfyoung can you make a separate issue about this. (and include that pointer)

@jreback
Copy link
Contributor

jreback commented Feb 24, 2016

thanks!

@jreback jreback closed this in bd1eebb Feb 24, 2016
@gfyoung gfyoung deleted the invalid_key_inform branch February 24, 2016 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.
3 participants
@gfyoung @jreback @jorisvandenbossche