gnu-octave / symbolic

A Symbolic Package for Octave using SymPy
https://octave.sourceforge.io/symbolic/
GNU General Public License v3.0
151 stars 36 forks source link

An Matrix can be incorrectly expanded into an Array #1228

Closed cbm755 closed 1 year ago

cbm755 commented 2 years ago

On my subs branch, this test is failing:

octave:6> test @sym/subsasgn
pydebug: make_matrix_or_array: making 2D Array...
pydebug: I am here with an array with shape (3, 5)
***** test
 % grow 2D
 b = 1:4; b = [b; 2*b];
 a = sym(b);
 rhs = [10 11; 12 13];
 a([1 end+1],end:end+1) = rhs;
 b([1 end+1],end:end+1) = rhs;
 assert(isequal( a, b ))
!!!!! test failed
assert (isequal (a, b)) failed

I'm not concerned at the moment about that isequal test failing (that is a different issue).

If you look at sympy(a) we see:

>> sympy(a)
ans = ImmutableDenseNDimArray(Tuple(Integer(1), Integer(2), Integer(3), Integer(10), Integer(11), Integer(2), Integer(4), Integer(6), Integer(8), Integer(0), Integer(0), Integer(0), Integer(0), Integer(12), Integer(13)), Tuple(Integer(3), Integer(5)))

But this is incorrect: it should've greedily become a Matrix...

alexvong243f commented 2 years ago

If we debug print the type of each element in make_matrix_or_array

--- a/inst/private/python_ipc_native.m
+++ b/inst/private/python_ipc_native.m
@@ -122,7 +122,9 @@ function [A, info] = python_ipc_native(what, cmd, varargin)
                     '    construct the corresponding Matrix.'
                     '    Otherwise, construct the corresponding 2D array.'
                     '    """'
-                    '    elts = itertools.chain.from_iterable(ls_of_ls)'
+                    '    elts = list(itertools.chain.from_iterable(ls_of_ls))'
+                    '    for elt in elts:'
+                    '        dbout(type(elt))'
                     '    if (dbg_no_array'
                     '        or all(isinstance(elt, Expr) for elt in elts)):'
                     '        return Matrix(ls_of_ls)'

and run this snippet

b = 1:4; b = [b; 2*b];
a = sym(b);
rhs = [10 11; 12 13];
a([1 end+1],end:end+1) = rhs;

We get the following output

pydebug: <class 'sympy.core.numbers.One'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'int'>
pydebug: <class 'int'>
pydebug: <class 'int'>
pydebug: <class 'int'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: <class 'sympy.core.numbers.Integer'>
pydebug: make_matrix_or_array: making 2D Array...

which shows that Array is being created due to isinstance(int, Expr) being False.

alexvong243f commented 2 years ago

My first thought was to improve our current isinstance(x, Expr) check to something better, but then I think this approach is basically duplicating the check done in sympy in a potentially buggy way.

Perhaps a better way is to try creating a Matrix first. If that fails, we catch the exception (turning warning to exception using technique similar to the one mentioned in #1194) and create an Array instead.

cbm755 commented 2 years ago

From your previous checking, it looks like non-sym are sneaking in somewhere: this is why isinstance(x, Expr) is false. I should maybe poke around and try to see why that is happening.

I do like "ask forgiveness not permission" in Python. That is:

try:
   make_matrix
except FooException:
   make_array 
cbm755 commented 2 years ago

traced this to a 0 that should be a S.Zero, see d3c83c02c6864ed3d23ca8b024ac29e6675f2927

(but I think you're right about longer term doing a try: except: later)

alexvong243f commented 1 year ago

Fixed in ef9744d1d63f0a517b121ea7e8c37b9a745fb55c