-
Notifications
You must be signed in to change notification settings - Fork 65
Add test on Series.getitem failure #619
base: master
Are you sure you want to change the base?
Conversation
What is the issue? getitem with negative slice is failing? |
S = pd.Series([11, 22, 33, 44, 55], index, name='A') | ||
ref_result = test_impl(S, start, end) | ||
jit_result = jit_impl(S, start, end) | ||
pd.testing.assert_series_equal(jit_result, ref_result) |
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.
Please use self.subTest context to verify each combination in a separate subtest.
Also I think there's no need to have additional test just for negative slices as they have the same implementation, so it's better to make existing test for int slices, i.e. test_series_getitem_idx_int_slice more generic, e.g:
slice_args = [0, 2, -2]
indexes_to_test = {
'int': [2, 3, 5, 6, 7],
'float': [2.0, 3, 5, 6, 7],
'string': ['2', '3', '5', '6', '7']
}
for start, end in product(slice_args, slice_args):
for index in indexes_to_test.values():
with self.subTest(start=start, stop=end, series_index=index):
S = pd.Series([11, 22, 33, 44, 55], index, name='A')
result_ref = test_impl(S, start, end)
result = jit_impl(S, start, end)
pd.testing.assert_series_equal(result, result_ref)
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.
@kozlov-alexey as far as I get, this is negative tests for negative slices. If it would be combined with positive slices we would need to skip all tests, instead of skiping specific ones.
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.
@AlexanderKalistratov What do you mean by negative tests? That an empty slice appear as a result? If so, that's true. Otherwise, I think it should not matter whether slice has negatives or positives from the test perspective (assuming it's a correct slice).
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.
@kozlov-alexey sounds reasonable. I thought this is some kind of numba limitation (negative slices). But it seems to be our own problem.
starts = [0, -2] | ||
ends = [-2, 0] | ||
indices = [[2, 3, 5, 6, 7], ['2', '3', '5', '6', '7'], ['2', '3', '5', '6', '7']] | ||
for start, end, index in zip(starts, ends, indices): |
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.
This would iterate over zipped sequences but until the end of the shortest one is found, so the last string index in indices won't be tested at all.
@akharche @AlexanderKalistratov The issue here is in the empty slice for the string series index only (other combinations seem to pass), i.e. original test fails on the second iteration, when start=-2, end=0 and index is ['2', '3', '5', '6', '7']. And the resulting slice should be:
|
def test_impl(S, start, end): | ||
return S[start:end] | ||
|
||
jit_impl = self.jit(test_impl) |
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.
It would probably be better to use the same name (hpat_func) everywhere.
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.
Very old PR. It is better to close it.
No description provided.