-
Notifications
You must be signed in to change notification settings - Fork 61
Fix for string tests failed due to bad str_overload #94
Fix for string tests failed due to bad str_overload #94
Conversation
This looks good. |
0501371
to
7c6ea14
Compare
hpat/str_ext.py
Outdated
# for strip methods define overloads to call Numba implementation | ||
# forwarding arg1 as a necessary 'chars' argument | ||
for method in str2str_1arg: | ||
func_text = "def str_overload(in_str, arg1):\n" |
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 is necessary to implement several function via auto generated source?
I would vote to make them as regular functions with required decorators.
@fschlimb What do you think about it? I mean, it might be better to stay away from such technique of programming.
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 usually try to avoid code duplication as much as possible. There will be cases where such code generation is required. So I tend to think it is not too bad.
Even though I am not sure I would now to do this with only decorators, such simpler cases can be handled with higher lever functions which call @overload. Here is a small POC:
from numba import njit, objmode, types
from numba.extending import overload
# dummy default python funcs
def x(a):
return a+a
def y(a):
return a*a
def z(a):
return a**a
# a function generically overloading a given function with one arg
def ovl(func):
@overload(func)
def _f(a):
def _ff(a):
with objmode(r='int32'):
r = func(a)
print(':) -> ', func(a))
return r
return _ff
# use the above overloader to overload x and y
ovl(x)
ovl(y)
# we can now use them in a jitted function
@njit
def z():
return x(4) + y(4)
r = z()
print('24 ?=', r)
I am not sure which one I like better. I am torn.
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.
@fschlimb I agree, code duplication is not good approach but autogeneration is also not good (I would say it is worse than code duplication). I would vote for any implementation that avoid code generation.
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.
fine with me as long as we are not duplicating code.
Just out of curiosity: why do you think code generation is bad? HPAT is a code-generator, numba is, and even python is. So this is not really unusual in this context.
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.
@fschlimb Because it non debuggable, non performance assessment, and etc. It only looks like a good approach but if something goes wrong it will be a real headache.
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.
@fschlimb We have one more option - generate the code at build stage, but avoid doing this in runtime.
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.
Frank, Sergey, thank you both for provided input!
I will try to re-write it using provided example, as to me it looks better than calling exec (both due to debugging and security considerations).
[BUG] Fixed problems with generation parquet files (IntelPython#93)
@kozlov-alexey Do you plan to make changes in this PR? |
@shssf Yes, I'm about to push it) |
Fixing issue with named series handling in fillna (IntelPython#95)
9f34326
to
fa5114f
Compare
As per suggestion above I've rewritten it using decorators and getitem.
|
PR #96 might help |
Fix for string tests failed due to bad str_overload
No description provided.