Use LLVMDoubleVisitor #110
Conversation
| def Lambdify(args, exprs, bool real=True, bool llvm=False): | ||
| IF HAVE_SYMENGINE_LLVM: | ||
| if llvm: | ||
| return LLVMDouble(args, exprs, real) |
There was a problem hiding this comment.
Should this raise an exception if llvm==True but HAVE_SYMENGINE_LLVM is False?
There was a problem hiding this comment.
I agree, at least a warning?
We could also leave it None and allow the user to choose default by setting an environment variable, e.g. SYMENGINE_LAMBDIFY_BACKEND or something along those lines? (we could then run the tests suite with
that variable set to llvm and lambda respectively)
There was a problem hiding this comment.
That's good idea, but llvm backend doesn't have all the features of lambda including complex support. Let's add that when llvm backend is on par with the lambda backend.
| @@ -50,6 +50,15 @@ def test_Lambdify(): | |||
| assert allclose(l(range(n, n+len(args))), | |||
| [3*n+3, n**2, -1/(n+2), n*(n+1)*(n+2)]) | |||
|
|
|||
| def test_Lambdify_LLVM(): | |||
| if not se.have_llvm: | |||
| return | |||
There was a problem hiding this comment.
And here we can test that exception.
| else: | ||
| self.lambda_double_complex[0].call(&out[0], &inp[0]) | ||
| cdef void _eval_complex(self, double complex[::1] inp, double complex[::1] out): | ||
| raise ValueError("Not supported") | ||
|
|
||
| # the two cpdef:ed methods below may use void return type | ||
| # once Cython 0.23 (from 2015) is acceptable as requirement. | ||
| cpdef unsafe_real(self, double[::1] inp, double[::1] out): |
There was a problem hiding this comment.
Since you now do size checking maybe the unsafe_ prefix doesn't really apply any more?
There was a problem hiding this comment.
This checking was already there in _eval which was called previously by unsafe_real. We can rename this to eval_real and this would call _eval_real (which should changed into a cpdef method)
| def Lambdify(args, exprs, bool real=True, bool llvm=False): | ||
| IF HAVE_SYMENGINE_LLVM: | ||
| if llvm: | ||
| return LLVMDouble(args, exprs, real) |
There was a problem hiding this comment.
I agree, at least a warning?
We could also leave it None and allow the user to choose default by setting an environment variable, e.g. SYMENGINE_LAMBDIFY_BACKEND or something along those lines? (we could then run the tests suite with
that variable set to llvm and lambda respectively)
isuruf
force-pushed
the
llvm
branch
4 times, most recently
from
5a1a347 to
b492f0b
Compare
|
Made changes. Ready for review again |
Thanks. Once this is merged, you can start on it. |
|
Thanks for the reviews |
|
+1 |
cc @bjodah