Added support for kwarg "use_array" in lambdify (fixes #2931) #2987
Conversation
…atrix in a more user friendly way
| @@ -167,6 +167,10 @@ def lambdify(args, expr, modules=None, printer=None, use_imps=True, dummify=True | |||
| to view the lambdified function or provide "sympy" as the module, you | |||
| should probably set dummify=False. | |||
|
|
|||
| If numpy is installed, the default behavior is to substitute Sympy Matrices | |||
| for numpy.matrix. If you would rather have a numpy.array returned, | |||
|
I'm for this option. Let's see if any others have a comment. I think this is a good initial solution. |
| @@ -145,7 +145,7 @@ def _import(module, reload="False"): | |||
|
|
|||
|
|
|||
| @doctest_depends_on(modules=('numpy')) | |||
| def lambdify(args, expr, modules=None, printer=None, use_imps=True, dummify=True): | |||
| def lambdify(args, expr, modules=None, printer=None, use_imps=True, dummify=True, use_array=False): | |||
There was a problem hiding this comment.
this line may need to be wrapped to 79 characters.
There was a problem hiding this comment.
Fixed. I was a little unsure how much sympy cared about pep 8 compliance, as some other lines were 80+ characters.
|
I'm +1 on this. Anyone else have comments? |
|
This is really handy, looks good to me. |
|
Any way we can get this merged? I'd like to start using this fix. |
|
Did you forget to push? All the comments where you say "fixed" are not fixed. |
Edited doc string for lambdify. Pep 8 formatting fixes.
|
Sorry about that. I'm new to version control, and because I started this on my master branch, things kind of got screwed up when I worked on other patches later. Looks like I accidentally deleted a commit. Everything should be fixed now though. Just waiting for the Travis tests to re-run. Note: this was already discussed on gitter, but I'm repeating here for others |
| f_arr = lambdify((x, y, z), A, use_array=True) | ||
| f_mat = lambdify((x, y, z), A) | ||
| numpy.testing.assert_allclose(f_mat(1, 2, 3), sol_mat) | ||
| numpy.testing.assert_allclose(f_arr(1, 2, 3), sol_arr) |
There was a problem hiding this comment.
I think these tests will work if f_mat and f_arr return matrices or arrays as it just checks whether the entries of the array are the same. It'd probably be a good idea to check whether the type of what f_arr and f_mat return are actually matrices and arrays. Maybe:
isinstance(f_arr, type(sol_arr))There was a problem hiding this comment.
Good catch. Should be good now. I left in the content checks as a just-in-case.
Now ensures use_array=True returns numpy.ndarray, and use_array=False returns numpy.matrix.
|
Hmmm, for some reason the tests passed on my machine, even though looking at the code now they shouldn't have. And I checked, I was running the tests on the same git-branch, and had saved beforehand. Any idea why this could happen? Anyway, should be good now. Hopefully. |
|
Ok, this one's good to go. |
Added support for kwarg "use_array" in lambdify (fixes #2931)
|
@jcrist Can you add this to the release notes: |
|
You know, thinking about this, and seeing the discussion that has been going on at numpy/numpy#4351, I think I now understand why np.matrix is so bad. We should probably do what is considered to be the best practice in the numpy community, and always use array, and just print matrix multiplication as np.dot in the lambdarepr printer. |
|
I like that idea. |
|
#7284. We should do this before the next release, since it implies reverting this change. |
Added kwarg in lambdify to support return of numpy array instead of matrix in a more user friendly way. Fixes #2931.