Skip to content

TST/CLN: correctly skip in indexes/common; add test for duplicated #21902

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

Merged
merged 4 commits into from
Aug 10, 2018
Merged

TST/CLN: correctly skip in indexes/common; add test for duplicated #21902

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f4175d
TST/CLN: correctly skip in indexes/common; add test for duplicated
h-vetinari Jul 13, 2018
4ab753a
Rename ambiguous test names
h-vetinari Jul 14, 2018
b6e2858
Incorporate review (jreback)
h-vetinari Jul 16, 2018
f9c9aab
Review; remove unhit instance-check
h-vetinari Aug 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
TST/CLN: correctly skip in indexes/common; add test for duplicated
  • Loading branch information
@h-vetinari
h-vetinari committed Aug 9, 2018
commit 0f4175d359afe01a3e40cd8ec1c7b0c7e0618f26
53 changes: 41 additions & 12 deletions pandas/tests/indexes/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def verify_pickle(self, indices):
def test_pickle_compat_construction(self):
# this is testing for pickle compat
if self._holder is None:
return
Copy link
Contributor

Choose a reason for hiding this comment

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

is this actually hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I didn't write these tests, and the inner workings of indexes/common.py are not immediately apparent (which files call it, what do they fill indices with, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to leave the bare return in there, if that's preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, put a breakpoint there and see if you would.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not able to do work on any of this for 2 weeks now. I'll have a look after.

pytest.skip('Skip check for uncertain type')

# need an object to create with
pytest.raises(TypeError, self._holder)
Expand Down Expand Up @@ -236,7 +236,7 @@ def test_set_name_methods(self, indices):

# don't tests a MultiIndex here (as its tested separated)
if isinstance(indices, MultiIndex):
return
pytest.skip('Skip check for MultiIndex')
original_name = indices.name
new_ind = indices.set_names([new_name])
assert new_ind.name == new_name
Expand Down Expand Up @@ -333,7 +333,8 @@ def test_copy_and_deepcopy(self, indices):
from copy import copy, deepcopy

if isinstance(indices, MultiIndex):
return
pytest.skip('Skip check for MultiIndex')

for func in (copy, deepcopy):
idx_copy = func(indices)
assert idx_copy is not indices
Expand All @@ -344,18 +345,46 @@ def test_copy_and_deepcopy(self, indices):

def test_duplicates(self, indices):
if type(indices) is not self._holder:
return
pytest.skip('Can only check if we have the correct type')
if not len(indices) or isinstance(indices, MultiIndex):
return
# MultiIndex tested separately in:
# tests/indexes/multi/test_unique_and_duplicates
pytest.skip('Skip check for empty Index and MultiIndex')

idx = self._holder([indices[0]] * 5)
assert not idx.is_unique
assert idx.has_duplicates

@pytest.mark.parametrize('keep', ['first', 'last', False])
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a duplicate these (pun intended) or are not testing indices duplicated currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback .duplicated is hardly ever tested directly, only indirectly for stuff like .drop_duplicates. Regardless of the changes to .duplicated in #21645, I think duplicated should be tested separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

so these are not relevant?

pandas/tests/indexes/common.py:    def test_duplicates(self, indices):
pandas/tests/indexes/test_category.py:    def test_duplicates(self):
pandas/tests/indexes/test_range.py:    def test_duplicates(self):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback No, test_duplicates (at least for pandas/tests/indexes/common.py, which is what this PR is about) tests .is_unique and .has_duplicates, but not the .duplicated-method itself.

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 point to the coverage that shows this is NOT tested?

Copy link
Contributor Author

@h-vetinari h-vetinari Jul 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jreback I never said that it is not tested, just that it is only tested implicitly. Any call to .drop_duplicates will invoke duplicated, so obviously the coverage works out.

def test_duplicated(self, indices, keep):
if type(indices) is not self._holder:
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 use isinstance here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback isinstance of what? I copied the checks from other tests, because I didn't know all the different cases that flow into common.py.

Copy link
Contributor

@jreback jreback Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

isinstance of self._holder, is this what we do elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not in indexes/common.py, as far as I can see. There's several isinstance of course, but never for self._holder, which can apparently be None as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback If I change

if type(indices) is not self._holder:

to

if not isinstance(indices, self._holder):

I get 9 failures (instead of skips), all of which are from

tests/indexes/test_numeric.TestInt64Index, when run against DatetimeIndex, PeriodIndex or TimedeltaIndex, mainly because of TypeError: Unsafe NumPy casting, you must explicitly cast it seems.

Copy link
Contributor Author

@h-vetinari h-vetinari Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jreback Seem to have found something: in my normal workbook, I get

pd.__version__
# '0.24.0.dev0+321.g0fe6ded52.dirty'
isinstance(pd.PeriodIndex, pd.Int64Index)
# False

but for some reason, within tests/indexes/common.py, this is the opposite:

    [...output of test run...]
    
    @pytest.mark.parametrize('keep', ['first', 'last', False])
    def test_duplicated(self, indices, keep):
        if not isinstance(indices, self._holder):
            pytest.skip('Can only check if we know the index type')
        if not len(indices) or isinstance(indices, (MultiIndex)):
            # MultiIndex tested separately in:
            # tests/indexes/multi/test_unique_and_duplicates
            pytest.skip('Skip check for empty Index and MultiIndex')
        if isinstance(indices, (PeriodIndex, DatetimeIndex)):
            # this branch should be impossible for Int64Index
            # after the instance-check above!
>           raise ValueError(f'{type(indices).__name__}, {self._holder.__name__}, '
                             f'{isinstance(indices, self._holder)}, {type(indices) is self._holder}')
E           ValueError: PeriodIndex, Int64Index, True, False

Same happens for DatetimeIndex. It does work for the original is not variant, so I'm leaving that as it is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened a follow-up: #22211

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I had gotten confused between instances and classes.

So DatetimeIndex, PeriodIndex and TimedeltaIndex are subclasses of Int64Index - but are unsafe to cast to Int64...? I guess this is intentional?

In any case, under these circumstances, I'm even more convinced that it's best to just stay with the if type(indices) is not self._holder: condition.

pytest.skip('Can only check if we know the index type')
if not len(indices) or isinstance(indices, MultiIndex):
# MultiIndex tested separately in:
# tests/indexes/multi/test_unique_and_duplicates
pytest.skip('Skip check for empty Index and MultiIndex')

idx = self._holder(indices)
if idx.has_duplicates:
# We need to be able to control creation of duplicates here
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is a bit obtuse, can you reword

# This is slightly circular, as drop_duplicates depends on
# duplicated, but in the end, it all works out because we
# cross-check with Series.duplicated
idx = idx.drop_duplicates()

n, k = len(idx), 10
duplicated_selection = np.random.choice(n, k * n)
expected = pd.Series(duplicated_selection).duplicated(keep=keep).values
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to check as a numpy array, much prefer to check the type and use assert_index_equal or assert_series_equal. is this how the other tests are?

Copy link
Contributor Author

@h-vetinari h-vetinari Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Um, index.duplicated() (without return_inverse) yields a numpy array - this is the documented signature (I guess because selecting on an Index really only needs an ndarray).

All the manual duplicated-tests actually create their own data, and know what the correct outcome should be. Here, we're feeding tons of different things through that test, so we need to determine - as I'm doing with duplicated_selection what is actually duplicate; self._holder(idx.values[duplicated_selection]) is then a duplicate Index of the correct type, but we know where its duplicates are (from inspecting duplicated_selection), and therefore can validate.

idx = self._holder(idx.values[duplicated_selection])

result = idx.duplicated(keep=keep)
tm.assert_numpy_array_equal(result, expected)

def test_unique(self, indices):
# don't test a MultiIndex here (as its tested separated)
# don't test a CategoricalIndex because categories change (GH 18291)
if isinstance(indices, (MultiIndex, CategoricalIndex)):
return
pytest.skip('Skip check for MultiIndex/CategoricalIndex')

# GH 17896
expected = indices.drop_duplicates()
Expand All @@ -375,7 +404,7 @@ def test_unique_na(self):
def test_get_unique_index(self, indices):
# MultiIndex tested separately
if not len(indices) or isinstance(indices, MultiIndex):
return
pytest.skip('Skip check for empty Index and MultiIndex')

idx = indices[[0] * 5]
idx_unique = indices[[0]]
Expand All @@ -394,7 +423,7 @@ def test_get_unique_index(self, indices):

# nans:
if not indices._can_hold_na:
return
pytest.skip('Skip na-check if index cannot hold na')

if needs_i8_conversion(indices):
vals = indices.asi8[[0] * 5]
Expand Down Expand Up @@ -423,7 +452,7 @@ def test_sort(self, indices):

def test_mutability(self, indices):
if not len(indices):
return
pytest.skip('Skip check for empty Index')
pytest.raises(TypeError, indices.__setitem__, 0, indices[0])

def test_view(self, indices):
Expand Down Expand Up @@ -761,7 +790,7 @@ def test_equals_op(self):
# GH9947, GH10637
index_a = self.create_index()
if isinstance(index_a, PeriodIndex):
return
pytest.skip('Skip check for PeriodIndex')

n = len(index_a)
index_b = index_a[0:-1]
Expand Down Expand Up @@ -989,11 +1018,11 @@ def test_searchsorted_monotonic(self, indices):
# not implemented for tuple searches in MultiIndex
# or Intervals searches in IntervalIndex
if isinstance(indices, (MultiIndex, IntervalIndex)):
return
pytest.skip('Skip check for MultiIndex/IntervalIndex')

# nothing to test if the index is empty
if indices.empty:
return
pytest.skip('Skip check for empty Index')
value = indices[0]

# determine the expected results (handle dupes for 'right')
Expand Down