Support for order kwarg in Lambdify #188

bjodah · 2017-09-19T12:49:35Z

This addresses gh-174.

bjodah · 2017-09-19T20:32:52Z

@raphaelquast I thought you might be interested. Any feedback (you can look at the test cases) is much appreciated.

bjodah · 2017-09-20T20:49:39Z

Running times for benchmarks/heterogeneous_output_Lambdify.py are pretty much unaffected. Before this PR:

SymEngine (lambda double)       speed-up factor (higher is better) vs sympy:       40.154
symengine (LLVM)                speed-up factor (higher is better) vs sympy:        306.3
symengine (ManualLLVM)          speed-up factor (higher is better) vs sympy:       297.43

Using this PR:

SymEngine (lambda double)       speed-up factor (higher is better) vs sympy:       40.659
symengine (LLVM)                speed-up factor (higher is better) vs sympy:       270.86
symengine (ManualLLVM)          speed-up factor (higher is better) vs sympy:       313.15

isuruf · 2017-09-20T22:22:03Z

I wouldn't worry much about LambdifyCSE as I'll get this PR finished before the next release and then we can get rid of sympy.

bjodah · 2017-09-20T22:28:27Z

I saw that one -- impressive!
The fix of LambdifyCSE here is actually only a minor part of the PR. The biggest change here is that it allows the user to specify order ('C' or 'F') which is passed to numpy.ravel and numpy.reshape. This gives the user control over whether extra dimensions should be added "on the right" or "on the left".

isuruf

I'm not sure whether we want order at both initialization and eval. Otherwise looks good to me.

isuruf · 2017-09-20T22:33:34Z

symengine/lib/symengine_wrapper.pyx

@@ -4399,16 +4365,20 @@ cdef class _Lambdify(object):
    cdef list out_shapes
    cdef readonly bint real
    cdef readonly int n_exprs
+    cdef readonly str order


Make this a char?

I tried that, but Cython seems to inject logic expecting an integer then.

isuruf · 2017-09-20T22:37:29Z

symengine/lib/symengine_wrapper.pyx

        self.numpy_dtype = np.float64 if self.real else np.complex128
-        self.out_shapes = [get_shape(expr) for expr in exprs]
+        self.out_shapes = [np.asarray(expr).shape for expr in exprs]


This is creating a numpy array just to get the shape. Isn't that expensive? I guess it doesn't matter much because this is in setup phase.

It could be more expensive, that's true. This was mostly the get rid of some non-trivial logic (the get_shape function). Maybe we should benchmark the creation of Lambdify instances as well.

bjodah · 2017-09-21T08:01:41Z

Since both args and exprs have numpy.ravel applied to them they are still converted to arrays. I changed the logic so that conversion only happens once now.

bjodah · 2017-09-21T14:55:56Z

One of the travis jobs needs a restart.

bjodah · 2017-09-22T11:51:39Z

Ok, I think I'm happy with this on my side. Anything else you'd like me to change @isuruf ?
Regarding the next release: any time plan for that?
Would there be interest in backporting this into a patch release for v0.3 (i.e. v0.3.1)?

EDIT: if one uses e.g. conda config --add channels symengine/label/dev, does that mean that I continuously get the master branch of python-symengine?

raphaelquast · 2017-09-22T15:43:58Z

@bjodah thanks a lot for getting this issue running!

I haven't had the time to check the changes so far,
but i definitely will and in case I encounter any troubles I'll let you know.

isuruf · 2017-09-22T16:15:37Z

symengine/lib/symengine_wrapper.pyx


-    def __cinit__(self, args, *exprs, cppbool real=True):
+    def __cinit__(self, args, *exprs, cppbool real=True, order='C'):


How about moving all the stuff from here to __init__ and remove storing args and exprs?

Nice catch. Works locally, pushed now.

isuruf · 2017-09-22T16:35:14Z

Regarding the next release: any time plan for that?

No plans yet. I was thinking of a release every six months.

EDIT: if one uses e.g. conda config --add channels symengine/label/dev, does that mean that I continuously get the master branch of python-symengine?

That was the initial plan, but we felt it's a waste of CI resources. So, whenever someone wants a new binary, a PR is made, like https://github.com/symengine/python-symengine-feedstock/pull/2/files

isuruf

Thanks for the PR. I'll merge when tests pass.

ShikharJ · 2017-09-22T18:47:20Z

Thanks for the PR.

raphaelquast · 2017-09-25T08:05:23Z

@isuruf

That was the initial plan, but we felt it's a waste of CI resources. So, whenever someone wants a
new binary, a PR is made, like https://github.com/symengine/python-symengine-feedstock/pull/2/files

unfortunately i don't fully get what you mean by that...
...in case i would like to test the new changes within my project, how do i get a
working binary (that incorporates the fixes) to replace my current installation of symengine?

bjodah added 3 commits September 19, 2017 14:48

Support for order kwarg in Lambdify

473679e

Simplify Lambdify's __init__

e949b3f

Support broadcasting also for CSE

93d2682

isuruf reviewed Sep 20, 2017

View reviewed changes

bjodah added 2 commits September 21, 2017 01:09

Remove order as kwarg from Lambdify.__call__

8376e3a

Make sure np.asanyarray is applied least number of times

248ac70

bjodah mentioned this pull request Sep 22, 2017

Test SymPy master bjodah/sym#19

Merged

isuruf reviewed Sep 22, 2017

View reviewed changes

Get rid of __cinit__ in lambdify

21f950d

isuruf approved these changes Sep 22, 2017

View reviewed changes

ShikharJ merged commit 3fd1e48 into symengine:master Sep 22, 2017

bjodah deleted the fix-lambdify branch September 25, 2017 09:14

bjodah mentioned this pull request Sep 25, 2017

Bump dev version symengine/python-symengine-feedstock#4

Merged

raphaelquast mentioned this pull request Oct 25, 2017

array-broadcasting in sp.lambdify, Lambdify and LambdifyCSE #174

Closed


		def __cinit__(self, args, *exprs, cppbool real=True):
		def __cinit__(self, args, *exprs, cppbool real=True, order='C'):

Support for order kwarg in Lambdify #188

Support for order kwarg in Lambdify #188

Uh oh!

Conversation

bjodah commented Sep 19, 2017

Uh oh!

bjodah commented Sep 19, 2017

Uh oh!

bjodah commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isuruf commented Sep 20, 2017

Uh oh!

bjodah commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isuruf left a comment

Choose a reason for hiding this comment

Uh oh!

isuruf Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

bjodah Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

isuruf Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

bjodah Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

bjodah commented Sep 21, 2017

Uh oh!

bjodah commented Sep 21, 2017

Uh oh!

bjodah commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raphaelquast commented Sep 22, 2017

Uh oh!

isuruf Sep 22, 2017

Choose a reason for hiding this comment

Uh oh!

bjodah Sep 22, 2017

Choose a reason for hiding this comment

Uh oh!

isuruf commented Sep 22, 2017

Uh oh!

isuruf left a comment

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Sep 22, 2017

Uh oh!

raphaelquast commented Sep 25, 2017

Uh oh!

Uh oh!

bjodah commented Sep 20, 2017 •

edited

Loading

bjodah commented Sep 20, 2017 •

edited

Loading

bjodah commented Sep 22, 2017 •

edited

Loading