Make *cat Array-compatible #1216
Conversation
inst/@sym/transpose.m
Outdated
| ' return x.T' | ||
| 'elif isinstance(x, NDimArray):' | ||
| ' xx = x.tolist()' | ||
| ' return Array(list(zip(*xx)))' |
There was a problem hiding this comment.
should these go through your new helper function? (not saying they should, just asking)
There was a problem hiding this comment.
Is it alright if I do
If isinstance(x, (MatrixBase, NDimArray)):
xx = x.tolist()
return make_matrix_or_array(list(zip(*xx)))
directly instead of splitting the case?
There was a problem hiding this comment.
This actually doesn't work because of issues with empty matrices. We have to split it into 2 cases.
There was a problem hiding this comment.
Replying to your original Q:
I thought about this but don't have a clear ans. Using make_matrix_or_array means sticking to a more consistent interface, while using Array is better for performance because no Expr checks are performed. I guess sticking to a more consistent interface is more important (?)
There was a problem hiding this comment.
As per my latest comment in #1124, we may want to use is_2d_sym and is_non_matrix_2d_sym for generality sake. WDYT?
There was a problem hiding this comment.
Yeah that sounds reasonable. A new PR perhaps
* 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 <gnu-octave#1215>. * inst/@sym/transpose.m: Implement it.
* inst/@sym/horzcat.m: Simplify it.
alexvong243f
force-pushed
the
make-cats-array-compat
branch
from
d104b60 to
0d93602
Compare
|
PR updated to fix issues raised, I think we can worry about generalizing to non- |
|
I agree for TableForm. One thing at a time |
This PR makes
@sym/vertcat,@sym/horzcatand@sym/transposeArray-compatible.In
@sym/horzcat, we make use of the property thatfor all
A_kwith compatible sizes.