-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN/TST: indexing/multiindex/test_getitem.py #24741
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24741 +/- ##
=======================================
Coverage 92.38% 92.38%
=======================================
Files 166 166
Lines 52358 52358
=======================================
Hits 48373 48373
Misses 3985 3985
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24741 +/- ##
==========================================
+ Coverage 92.38% 92.39% +<.01%
==========================================
Files 166 166
Lines 52358 52358
==========================================
+ Hits 48373 48374 +1
+ Misses 3985 3984 -1
Continue to review full report at Codecov.
|
@@ -15,6 +16,14 @@ def single_level_multiindex(): | |||
codes=[[0, 1, 2, 3]], names=['first']) | |||
|
|||
|
|||
@pytest.fixture | |||
def frame_random_data_integer_multi_index(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fixture duplicative of the one in conftest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd : probably yes.. it is a dataframe with random data with an integer multi-level index, so multiindex_year_month_day_dataframe_random_data
fixture could probably be used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - if you want to tackle that in a follow up always good to reduce duplication
(pd.IndexSlice[:, ['foo']], True, None), | ||
(pd.IndexSlice[:, ['foo', 'bah']], True, None) | ||
]) | ||
def test_getitem_duplicates_multiindex_missing_indexers(indexer, is_level1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's better to rename these get_loc
since that is in fact what they are testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd agreed. this PR is cleaning up test_getitem.py after splitting and parametrizing. that task is still to do for test_loc.py so the renaming can be done in that cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have renamed tests moved to test_loc.py to include loc_getitem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. ex @WillAyd commets
Thanks @simonjayhawkins keep them coming! |
follow-on from #24452 to add section breaks and move tests. also
KeyError
checks made more explicit.