Skip to content
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

An Matrix can be incorrectly expanded into an Array #1228

Closed
cbm755 opened this issue Sep 4, 2022 · 5 comments
Closed

An Matrix can be incorrectly expanded into an Array #1228

cbm755 opened this issue Sep 4, 2022 · 5 comments
Assignees

Comments

@cbm755
Copy link
Collaborator

cbm755 commented Sep 4, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

cbm755 commented Sep 4, 2022

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 added a commit that referenced this issue Sep 5, 2022
@cbm755
Copy link
Collaborator Author

cbm755 commented Sep 5, 2022

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

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

cbm755 added a commit that referenced this issue Sep 6, 2022
@alexvong243f
Copy link
Collaborator

Fixed in ef9744d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants