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.

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

Raised value error on incorrect na_option #22037

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Raised value error on incorrect na_option #22037

merged 2 commits into from
Jul 25, 2018

Conversation

raguiar2
Copy link
Contributor

@raguiar2 raguiar2 commented Jul 24, 2018 •
edited by WillAyd
Loading

WillAyd
WillAyd requested changes Jul 24, 2018
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Just need test(s) to ensure the behavior. Can also add a whatsnew for 0.24

@WillAyd WillAyd added the Error Reporting Incorrect or improved errors from pandas label Jul 24, 2018
@WillAyd
Copy link
Member

WillAyd commented Jul 24, 2018

Also can you make sure this works for all of Series, DataFrame and GroupBy objects? Behavior should be identical across all three

@codecov
Copy link

codecov bot commented Jul 24, 2018 •
edited
Loading

Codecov Report

Merging #22037 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22037      +/-   ##
==========================================
+ Coverage      92%      92%   +<.01%     
==========================================
  Files         170      170              
  Lines       50553    50556       +3     
==========================================
+ Hits        46510    46513       +3     
  Misses       4043     4043
Flag Coverage Δ
#multiple 90.4% <ø> (ø) ⬆️
#single 42.21% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/generic.py 96.47% <ø> (ø) ⬆️

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 2d0c961...59c98a6. Read the comment docs.

@raguiar2
Copy link
Contributor Author

Not sure why travis-ci is failing. Looks unrelated/might have to do with an upstream dependency? Do I have to create a new PR or can I re-run the build somehow?

WillAyd
WillAyd requested changes Jul 25, 2018
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

One comment (applicable to all tests) but otherwise LGTM. Travis failures look unrelated so will see on re-push if resolved, but otherwise I don't think it's a blocker for this PR

pandas/tests/frame/test_rank.py Outdated
with pytest.raises(ValueError) as excinfo:
self.frame.rank(na_option='bad', ascending=False)

assert msg == str(excinfo.value)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this you can pass the expected msg as an argument to tm.assert_raises_regex

@raguiar2 raguiar2 force-pushed the ra--add-value-error-to-rank branch from 9b8c978 to 59c98a6 Compare July 25, 2018 07:18
@raguiar2
Copy link
Contributor Author

@WillAyd PTAL!

WillAyd
WillAyd approved these changes Jul 25, 2018
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

This lgtm - @jreback any feedback before I merge?

@jreback
Copy link
Contributor

jreback commented Jul 25, 2018

lgtm

do we have an open issue for this?

@WillAyd
Copy link
Member

WillAyd commented Jul 25, 2018

Updated original comment to make linkage clearer. Merging later today if no objections

@TomAugspurger TomAugspurger merged commit 600b9c8 into pandas-dev:master Jul 25, 2018
@TomAugspurger
Copy link
Contributor

Thanks @raguiar2!

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@raguiar2
Raised value error on incorrect na_option (pandas-dev#22037)
043cd5b 
* added value error on incorrect na_option

* added test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd approved these changes

Assignees
No one assigned
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rank 'na_option="bottom"' Usage Clarification
4 participants
@raguiar2 @WillAyd @jreback @TomAugspurger