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.
Please extract duplicated code into separate function.
There was a problem hiding this comment.
@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.
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.
Is it possible to avoid autogeneration of the code and exec() method?
There was a problem hiding this comment.
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.
Do you think it will faster? Or why you changed the order?
There was a problem hiding this comment.
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.
Do we want to check the case if a=b and evaluate properly throw exception?
There was a problem hiding this comment.
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.
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.
Does it supports bin, oct and hex numbers? Scientific notation (0.5E-01)?
There was a problem hiding this comment.
@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.
kozlov-alexey
force-pushed
the
feature/add_missed_tests1
branch
from
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.