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

CLN: raise correct error for Panel sort_values #16532

Merged
merged 4 commits into from
May 31, 2017

Conversation

pepicello
Copy link
Contributor

@pepicello pepicello commented May 29, 2017

I do not believe there is a need for tests or whatsnew entry.

I modified the error raised (I think NotImplementedError should reflect better the situation) and added a small docstring to explain that the method is not implemented (similar to other methods in the same class)

@codecov
Copy link

codecov bot commented May 29, 2017

Codecov Report

Merging #16532 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16532      +/-   ##
==========================================
+ Coverage   90.79%    90.8%   +<.01%     
==========================================
  Files         161      161              
  Lines       51063    51062       -1     
==========================================
  Hits        46365    46365              
+ Misses       4698     4697       -1
Flag Coverage Δ
#multiple 88.64% <100%> (ø) ⬆️
#single 40.15% <100%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 92.31% <100%> (+0.04%) ⬆️

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 e60dc4c...4d9a956. Read the comment docs.

@codecov
Copy link

codecov bot commented May 29, 2017

Codecov Report

Merging #16532 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16532      +/-   ##
==========================================
- Coverage   90.79%   90.75%   -0.05%     
==========================================
  Files         161      161              
  Lines       51063    51070       +7     
==========================================
- Hits        46365    46348      -17     
- Misses       4698     4722      +24
Flag Coverage Δ
#multiple 88.59% <100%> (-0.05%) ⬇️
#single 40.11% <100%> (-0.05%) ⬇️
Impacted Files Coverage Δ
pandas/core/generic.py 92.31% <100%> (+0.04%) ⬆️
pandas/io/formats/terminal.py 16.43% <0%> (-31.39%) ⬇️
pandas/core/window.py 96.49% <0%> (+0.01%) ⬆️

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 e60dc4c...c5d81ee. Read the comment docs.

@jreback
Copy link
Contributor

jreback commented May 30, 2017

since we are explicit on this error, can you add a test for Panel that check this. otherwise lgtm.

@jreback jreback added this to the 0.21.0 milestone May 30, 2017
@jreback jreback added Error Reporting Incorrect or improved errors from pandas Panel labels May 30, 2017
@pepicello pepicello force-pushed the correct_docs_for_missing_feature branch from 4d9a956 to 4b9a508 Compare May 30, 2017 22:49
@@ -2429,6 +2429,10 @@ def test_all_any_unhandled(self):
pytest.raises(NotImplementedError, self.panel.all, bool_only=True)
pytest.raises(NotImplementedError, self.panel.any, bool_only=True)

def test_sort_values(self):
pytest.raises(NotImplementedError, self.panel.sort_values)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the issue reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done



def test_sort_values(self):
pytest.raises(NotImplementedError, self.panel4d.sort_values)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@jreback
Copy link
Contributor

jreback commented May 30, 2017

minor comments. lgtm. ping on green.

@pepicello
Copy link
Contributor Author

Makes sense, thanks. Tests added. In hindsight, I have re-added the arguments in the function (making the first argument optional) so that even if the method is called with arguments, as it is done for DataFrames, it still throws a NotImplementedError rather than a TypeError.

@jreback jreback merged commit ee8346d into pandas-dev:master May 31, 2017
@jreback
Copy link
Contributor

jreback commented May 31, 2017

thanks!

Kiv pushed a commit to Kiv/pandas that referenced this pull request Jun 11, 2017
stangirala pushed a commit to stangirala/pandas that referenced this pull request Jun 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sort_values not implemented for Panel
2 participants