-
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
Open
cbm755
wants to merge
40
commits into
main
Choose a base branch
from
Array_not_Matrix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
880b600
Use a SymPy Array not a Matrix for non-Expr
cbm755 626c1a6
Python header: Add new function 'make_matrix_or_array'.
cdcc561
@sym/private/mat_rclist_access.m: Test Array-compatible code.
c18f830
@sym/private/mat_rclist_asgn.m: Test Array-compatible code.
10d26f2
@sym/{horzcat,vertcat}: Fix typo.
5f171e7
@sym/vertcat: Make function Array-compatible.
34c1f52
@sym/transpose: Make function Array-compatible.
6bfddcd
@sym/horzcat: Make function Array-compatible by simplifying it.
ce23c11
@sym/private/mat_rclist_{access,asgn}: Remove unused variables.
f95648e
@sym/repmat: Re-implement function without using 'pycall_sympy__'.
4a74fe2
elementwise_op: use 2d loop for Array
cbm755 0525456
private/python_ipc_native: Disable 'dbg_no_array' to keep in sync...
883326b
uniop_bool_helper: Array support in "bool" case
cbm755 5cb250e
logical: remove 1-d indexing for Array support
cbm755 d645f33
Minor fix to test: ctranspose of logical is tested elsewhere
cbm755 e3e0abc
Oops MatrixBase
cbm755 7386556
isAlways: avoid 1-d matrix/Array indexing
cbm755 af8caaa
levels=1 sufficient with tolist() from 2dArray/Matrix
cbm755 2ec6f02
subs: use make_matrix_or_array and use 2-d indexing
cbm755 978d8f0
Use S.Zero not integer 0
cbm755 535ecae
Array: fix a failing test in subasgn
cbm755 6cc099b
@sym/vertcat: Use sympy function 'flatten'.
f51d8ad
@sym/private/mat_rclist_asgn: Use sympy function 'flatten'.
84962e8
@sym/transpose: Use sympy function 'transpose'.
fd7efde
@sym/private/mat_replace: Make matrix mutable before modifying.
c154b6d
make_matrix_or_array: Generalise to accept an iterable of iterables.
25e0ebc
isconstant: fix behaviour with Array and add tests for errors
cbm755 60d5c12
find: work with Array
cbm755 6b02735
Fix some unused code
cbm755 d0ab2a3
elementwise_op: call make_array_or_matrix
cbm755 ea977c3
Rough gating of Array on older SymPy
cbm755 a90f041
Python header: Rename 'make_matrix_or_array' -> 'make_2d_sym'.
27b64eb
make_2d_sym: support flat input and shape
cbm755 174cbdc
Workaround nsolve with immutable x vars
cbm755 ee92b66
subs: after Array port was creating incorrect empties
cbm755 6f06abb
vertcat: tests for empty cases
cbm755 edcaec2
vertcat: use a flat list and shape for correct empty
cbm755 90af895
horzcat/vertcat: add tests for horzcat as well
cbm755 c9252ee
Port helpers from list-of-lists to flat+shape
cbm755 47834bd
isntance doesn't need special None handling
cbm755 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 withipc-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 ofpycall_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:To implement horzcat in terms of vertcat and transpose, we do it inside the octsmypy python library:
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: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.