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

Implemented Ellipsis in slicing Sympy Matrices. #11392

Closed
wants to merge 2 commits into from

Conversation

arihantparsoya
Copy link
Contributor

This is fix for #11345. Added conditions when the argument is ellipsis.

Sample Program

>>> from sympy import *
>>> from sympy.abc import m, n
>>> X = MatrixSymbol('X', n, m)
>>> X[...]
X[:n, :m]
>>> X[2, ...]
X[2, :m]
>>> X[..., 2]
X[:n, 2]

fix for  sympy#11345.

Added test cases for Ellipsis arguments for matrix slicing.
… means the same as : in a matrix. Hence, if the argument is ‘….’ ,
input to MatrixSlice is set to `slice(None, None, None) `.
@arihantparsoya
Copy link
Contributor Author

@leosartaj , can your review this?

@arihantparsoya
Copy link
Contributor Author

@asmeurer , can you review this?

@leosartaj
Copy link
Member

This does not work.

>>> m = Matrix([[1, 2, 3], [4, 5, 6]])
>>> m[...]
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-5-0ccb1cae6e36> in <module>()
----> 1 m[...]

/home/leosartaj/anaconda/lib/python2.7/site-packages/sympy/matrices/dense.pyc in __getitem__(self, key)
     94             if isinstance(key, slice):
     95                 return self._mat[key]
---> 96             return self._mat[a2idx(key)]
     97 
     98     def __setitem__(self, key, value):

/home/leosartaj/anaconda/lib/python2.7/site-packages/sympy/matrices/matrices.pyc in a2idx(j, n)
   4412             j = j.__index__()
   4413         except AttributeError:
-> 4414             raise IndexError("Invalid index a[%r]" % (j, ))
   4415     if n is not None:
   4416         if j < 0:

IndexError: Invalid index a[Ellipsis]

@leosartaj
Copy link
Member

This also doesn't work as expected.

>>> m = MatrixSymbol('A', 3, 4)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-16-0ccb1cae6e36> in <module>()
----> 1 m[...]

/home/leosartaj/anaconda/lib/python2.7/site-packages/sympy/matrices/expressions/matexpr.pyc in __getitem__(self, key)
    252                 raise IndexError("Single index only supported for "
    253                                  "non-symbolic indices.")
--> 254         raise IndexError("Invalid index, wanted %s[i,j]" % self)
    255 
    256     def as_explicit(self):

IndexError: Invalid index, wanted A[i,j]

I would expect something like this

>>> m[...]
A[:3, :4]

@arihantparsoya
Copy link
Contributor Author

@leosartaj , the following program does work in my local computer:

>>> from sympy import *
>>> m = MatrixSymbol('A', 3, 4)
>>> m[...]
A[:3, :4]

@arihantparsoya
Copy link
Contributor Author

@leosartaj , the following program works now:

>>> from sympy import *
>>> m = Matrix([[1, 2, 3], [4, 5, 6]])
>>> m[...]
Matrix([
[1, 2, 3],
[4, 5, 6]])
>>> m[0,...]
Matrix([[1, 2, 3]])

@arihantparsoya
Copy link
Contributor Author

@leosartaj , ping.

@arihantparsoya
Copy link
Contributor Author

@asmeurer , ping.

1 similar comment
@arihantparsoya
Copy link
Contributor Author

@asmeurer , ping.

i = j = slice(None, None, None)
else:
i, j = key
if (i is Ellipsis) or (j is Ellipsis):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line does not have any effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing this PR. If only one among i or j is Ellipsis, then this line will have use.

Example

assert X[2, ...] == MatrixSlice(X, 2, (0, m))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjodah is correct. the four lines below handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have fixed it.

@arihantparsoya
Copy link
Contributor Author

@leosartaj , ping.

@@ -451,6 +451,9 @@ def test_slicing():
m2 = Matrix([[0, 1, 2, 3], [4, 5, 6, 7], [8, 9, 10, 11], [12, 13, 14, 15]])
assert m2[:, -1] == Matrix(4, 1, [3, 7, 11, 15])
assert m2[-2:, :] == Matrix([[8, 9, 10, 11], [12, 13, 14, 15]])
assert m2[...] == m2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't equal to m2[:] right? In other cases, ... and : are equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, its not equal.

… means the same as : in a matrix. Hence, if the argument is ‘….’ ,
input to MatrixSlice is set to `slice(None, None, None) `.
@sylee957
Copy link
Member

Although I don't see any clear advantage of slicing with ellipsis in matrix,
I see ellipsis slicing is implemented for numpy arrays and it can follow the same convention for sympy array.
So I think that this should be implemented first to sympy NDimArray and rather backported from there for sympy matrix.

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

Successfully merging this pull request may close these issues.

None yet

6 participants