-
Notifications
You must be signed in to change notification settings - Fork 61
Add tests for Series arithmetic and comparison methods and fix Series.div #114
Add tests for Series arithmetic and comparison methods and fix Series.div #114
Conversation
[BUG] Fixed problems with generation parquet files (IntelPython#93)
Fixing issue with named series handling in fillna (IntelPython#95)
Merge from public
Merging commits from public repo
Merge from public repo
Merge from public repo
Merge from public repo
A += i | ||
return A | ||
hpat_func = hpat.jit(test_impl) | ||
arithmetic_binops = ('+', '-', '*', '/', '//', '%', '**') |
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 extract duplicated code into separate function.
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.
@shssf Sorry, I don't understand what duplication has to be removed, do you mean combine all tests for arithmetic operations (i.e. op1 - op3) into one single tests?
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.
Not in one test but these tests have many similar code. The idea is to make this code as separate function which could be called from these tests.
hpat/tests/test_series.py
Outdated
test_impl(df.A, 1), check_names=False) | ||
for operator in arithmetic_binops: | ||
func_text = "def test_impl(S, i):\n" | ||
func_text += " return S {} i\n".format(operator) |
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 it possible to avoid autogeneration of the code and exec()
method?
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.
Not sure if decorator-solution works here, as we use operators in the function body.
Will check, but I don't really think it's a problem, because it's not in functional code, but in tests and they seem to be simpler with this form.
operator.ge: 'ge', | ||
operator.ne: 'ne', | ||
operator.eq: 'eq', | ||
'add': operator.add, |
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.
Do you think it will faster? Or why you changed the order?
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.
No, the order was changes because there were two values with the same key in original explicit_binop_funcs:
operator.truediv: 'div',
operator.truediv: 'truediv',
Hence iterating over (key, value) pairs didn't work and only one (truediv) Series method was generated, so Series.div didn't work at all - it failed with:
numba.errors.TypingError: Failed in hpat mode pipeline (step: nopython frontend)
Unknown attribute 'div' of type series(int64, array(int64, 1d, C), none, False)
A = pd.Series(np.arange(n)) | ||
B = pd.Series(np.arange(n)**2) | ||
pd.testing.assert_series_equal(hpat_func(A, B), test_impl(A, B)) | ||
comparison_binops = ('<', '>', '<=', '>=', '!=', '==') |
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.
Do we want to check the case if a=b
and evaluate properly throw exception?
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.
Hmm, I didn't think of it. But it as far as I understand it won't test HPAT anyway, it will test Python, because SyntaxError will be raised before we attempt to jit-compile anything. Am I wrong?
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 don't know. I just propose the idea to implement additional checks. It is ok if you think it is not applicable here.
hpat/tests/test_series.py
Outdated
return A.abs() | ||
hpat_func = hpat.jit(test_impl) | ||
|
||
S = pd.Series([1, 2.2, -5.8, 3, 2, -1.2, -4, 4.7]) |
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.
Does it supports bin, oct and hex numbers? Scientific notation (0.5E-01)?
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.
@shssf I overlooked it, there's no need in this test at all - there is another one test_series_abs1, which also checks NaN. Anyway, it works, yes. I will add hex/dec/oct literals to it.
7ff9df4
to
3867c56
Compare
For me this auto generation looks too complicated. |
….div (IntelPython#114) * Fixing a bug in implementation of Series.div via binary operator * More tests added for Series arithmetic and comparison methods * Applying review comments
No description provided.