-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
draft: Use a SymPy Array not a Matrix for non-Expr #1194
base: main
Are you sure you want to change the base?
Conversation
@alexvong1995 here's a half-baked idea for Issue #1124: roughly when we have non- |
@alexvong1995 What do you think of this? Basically in a few places (probably not that many) we return Array instead of Matrix. Specifically we return Matrix greedily whenever everything is a Expr, else Array. (It will need fixes upstream but I'm less worried about that.) Maybe you can try doing this in the |
I think the the test failure is partly due to I'm not sure what to do about it. It seems there's more code in |
that could be true: I haven't looked at pythonic much recently. You can always comment out "dbout()" (it is just debugging). With Pythonic you can probably just Anyway, I think even concentrating on |
@alexvong1995 you can rebase and force push this on top of main (if you like). |
See gnu-octave#1194 for more information. * inst/@sym/private/mat_rclist_access.m: Test it.
See gnu-octave#1194 for more information. * inst/@sym/private/mat_rclist_asgn.m: Test it.
I think what we should do next is to make |
33c21bb
to
a7c44c0
Compare
------- Original Message -------
On Monday, August 29th, 2022 at 12:04 AM, Colin B. Macdonald ***@***.***> wrote:
@alexvong1995 you can rebase and force push this on top of main (if you like).
rebased onto main and force pushed as a7c44c0
… —
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
end | ||
h = pycall_sympy__ (cmd, varargin{:}); | ||
args = cellfun (@transpose, varargin, 'UniformOutput', false); | ||
h = vertcat (args{:}).'; |
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.
while I think of it: I approve of this kind of "surface decrease" (having fewer functions that call pycall_sympy__
) BUT it is important to be aware that there is a downside: namely more "roundtrips" between Python and Octave. This is most noticeable with ipc-system
but even with popen2, there are additional copies being made as everything is serialized back-and-forth over the pipe.
Its analogous to HTTP where all objects keep getting converted back-and-forth to JSON.
No action required but its something to keep in mind: perfect software design here might blow-up the latency and IO costs.
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.
Interesting thoughts!
Normally when working with a single language, the classic solution is to use an (optimizing) compiler to optimize away all the redundant conversions. Unfortunately, this doesn't work for us because of language barrier: a compiler can't optimize across language boundary :(
Some of my thoughts:
First, pythonic may have already solved our problem! While I am not familiar with the internals of pythonic, perhaps using pythonic's py.
... instead of pycall_sympy__
can avoid the needless conversions when going through a pipe. Of course, we should benchmark to see if it's indeed the case.
I believe this was the original future subproject but we changed it to the current (more urgent) one.
Another way is to create a octsmypy python library which contains one python function for each m-file function.
For example, say we want to implement m-file function @sym/transpose
, @sym/vertcat
and @sym/horzcat
. Then our octsmypy python library should contain them as well. More precisely, we can do:
from octsympy.sym import transpose, vertcat, horzcat
To implement horzcat in terms of vertcat and transpose, we do it inside the octsmypy python library:
def horzcat(*XX):
return transpose(vertcat(*[transpose(X) for X in XX]))
without the redundant conversions.
Finally, the m-file functions @sym/transpose
, @sym/vertcat
and @sym/horzcat
are now thin wrappers over the corresponding octsympy python library functions, such as:
function h = vertcat (varargin)
args = ...
h = pycall_sympy__ ('from return octsympy.sym.vertcat(*_ins)', args{:});
end
and so on...
Perhaps this can be mentioned in the final report as possible future projects (?)
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 agree Pythonic offers possibilities. I think it currently still does conversion: a 'sym' is really just a struct of the 'srepr' (string representation) from SymPy, indep of IPC mechanism. But your 'xsym' idea would change that.
Agree about mentioing these things ad future projects.
Another thing to mention: CI pythonic runs are twice as slow as Popen2. I susect there migit be some 'low hanging fruit' in the Pythonic project.
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.
And just for the record: another alternative that requires very little effort is (a) being mindful of this issue and aiming for 1 (or a small number) of language crosses per function (b) balancing that against technical debt of code duplication etc. This is the approach we've been using so far.
I think there was a way to turn an warning into an exception in Python. That might be useful when debugging here... like |
------- Original Message -------
On Friday, September 2nd, 2022 at 4:57 PM, Colin B. Macdonald ***@***.***> wrote:
I think there was a way to turn an warning into an exception in Python. That might be useful when debugging here... like `SymPyDeprecationWarning` becomes an exception.
# The following example works for me:
from sympy import Matrix
from sympy.utilities.exceptions import SymPyDeprecationWarning
import warnings
warnings.filterwarnings("error", category=SymPyDeprecationWarning)
Matrix([[True]])
# I figure this out after reading multiple issues https://github.com/sympy/sympy/issues?q=is%3Aissue+is%3Aopen+SymPyDeprecationWarning of sympy.
# It would be good to know if this is documented somewhere or not.
… —
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
f0e5ecd
to
f04314e
Compare
It has mostly the same semantics as Matrix but can contain more general things. There might be some issues about different shapes of empties though... WIP on a fix for #1052.
Fixes #1211. * inst/private/python_header.py: Add it. * inst/private/python_ipc_native.m: Add it.
See #1194 for more information. * inst/@sym/private/mat_rclist_access.m: Test it.
See #1194 for more information. * inst/@sym/private/mat_rclist_asgn.m: Test it.
* inst/@sym/{horzcat,vertcat}.m: Fix them.
* inst/@sym/vertcat.m: Implement it.
This is a prerequisite for making @sym/horzcat Array-compatible. Partially fixes <#1215>. * inst/@sym/transpose.m: Implement it.
* inst/@sym/horzcat.m: Simplify it.
* inst/@sym/private/mat_rclist_{access,asgn}.m: Remove them.
The function is now compatible with non-Matrix 2D sym (e.g. Array, TableForm). Also, it now outputs empty matrices in the same way as upstream Octave (fixes #1218). * inst/@sym/repmat.m: Re-implement it and add tests accordingly.
...with 'inst/private/python_header.py'. Follow-up to 56bd719. * inst/private/python_ipc_native.m: Disable it.
This routine is supposed to return a logical Octave array. Do linear indexing on the flattened tolist output which will work on both Array and Matrix. Fixes #1221.
In fact it is failing elsewhere but the point of *this* test is to change the orientation of a vector.
Fixes #1228.
Follow-up to d3b1547. * inst/@sym/vertcat.m: Use it.
* inst/@sym/private/mat_rclist_asgn.m: Use it.
Follow-up to 136184d. * inst/@sym/transpose.m: Use it.
* inst/private/{python_header.py,python_ipc_native.m}: Generalise function 'make_matrix_or_array', use sympy function 'flatten' in 'make_matrix_or_array'.
This was broken with syntax errors since 2016... Apply fix for Array but leave a comment that this is unused and untested. It is referred to in isprime but commented out.
Instead of doing it ourselves, prepare a list of lists and call the centralized helper function.
Generalises 'make_matrix_or_array' so that we can choose to represent non-Matrix 2D sym to be something other than an Array in the future. For example, we could use TableForm. * inst/private/{python_header.py,python_ipc_native.m}: Rename function 'make_matrix_or_array' -> 'make_2d_sym', variable 'dbg_no_array' -> 'dbg_matrix_only' and adjust accordingly. * inst/@sym/private/mat_rclist_{access,asgn}.m: Adjust accordingly. * inst/@sym/vertcat.m: Adjust accordingly.
Fixes #1236. For now, just add a new underscore version that takes a shape kwarg.
Related to #1236. I don't see a problem with the list-of-lists approach, but these are the only routine using it so we can delete some code if we port them to flat + shape.
4f6b65b
to
47834bd
Compare
It has mostly the same semantics as Matrix but can contain more general
things. There might be some issues about different shapes of empties
though... WIP on a fix for #1052 and #1055.