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

Modified behavior of __getitem__ for arrays #17226

Merged
merged 4 commits into from Jul 25, 2019

Conversation

@kangzhiq
Copy link
Contributor

kangzhiq commented Jul 19, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

  • Changed the way with which the arrays access elements. Taking
    examples from NumPy
  • Modified tests to avoid: A[0], this kind of access to item
  • Modified accordingly the functions in which getitem is being used

Before:

>>> from sympy import Array
>>> A = Array([[1, 2, 3], [4, 5, 6], [7, 8, 9]])
>>> A[0, 0]
1
>>> A[0]
1

Now:

>>> from sympy import Array
>>> A = Array([[1, 2, 3], [4, 5, 6], [7, 8, 9]])
>>> A[0, 0]
1
>>> A[0]
[1, 2, 3]

Other comments

Release Notes

  • tensor
    • Modified the behavior of __getitem__ on Array to better match NumPy
- Changed the way with which the arrays access the elements. Taking
example of NumPy
- Modified tests to avoid: A[0], this kind of accessing item
- Modified accordingly the functions in which __getitem__ is being used
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Jul 19, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • tensor
    • Modified the behavior of __getitem__ on Array to better match NumPy (#17226 by @kangzhiq)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
- Changed the way with which the arrays access elements. Taking
examples from NumPy
- Modified tests to avoid: A[0], this kind of access to item
- Modified accordingly the functions in which __getitem__ is being used

Before:
```python
>>> from sympy import Array
>>> A = Array([[1, 2, 3], [4, 5, 6], [7, 8, 9]])
>>> A[0, 0]
1
>>> A[0]
1
```
Now:
```python
>>> from sympy import Array
>>> A = Array([[1, 2, 3], [4, 5, 6], [7, 8, 9]])
>>> A[0, 0]
1
>>> A[0]
[1, 2, 3]
```
#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* tensor
  * Modified the behavior of `__getitem__` on Array to better match NumPy  
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@sylee957

This comment has been minimized.

Copy link
Member

sylee957 commented Jul 19, 2019

If the array structure is changed, you may also have to change things like length, iterable to make it work.

>>> from sympy import Array
>>> A = Array([[1, 2, 3], [4, 5, 6], [7, 8, 9]])
>>> len(A)
9
>>> [*A]
[1, 2, 3, 4, 5, 6, 7, 8, 9]

Python list of lists structure should give

>>> A = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
>>> len(A)
3
>>> [*A]
[[1, 2, 3], [4, 5, 6], [7, 8, 9]]
@kangzhiq

This comment has been minimized.

Copy link
Contributor Author

kangzhiq commented Jul 20, 2019

Tests are failing because of this issue: #17230

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 20, 2019

Codecov Report

Merging #17226 into master will increase coverage by 4.072%.
The diff coverage is 96.551%.

@@              Coverage Diff              @@
##            master    #17226       +/-   ##
=============================================
+ Coverage   70.501%   74.574%   +4.072%     
=============================================
  Files          623       623               
  Lines       161580    161803      +223     
  Branches     37925     37974       +49     
=============================================
+ Hits        113917    120663     +6746     
+ Misses       42241     35791     -6450     
+ Partials      5422      5349       -73
@sylee957 sylee957 added the tensor label Jul 20, 2019
@@ -164,7 +179,7 @@ def _new(cls, iterable, shape, **kwargs):
self._shape = shape
self._array = list(flat_list)
self._rank = len(shape)
self._loop_size = functools.reduce(lambda x,y: x*y, shape) if shape else 0
self._loop_size = functools.reduce(lambda x,y: x*y, shape) if shape else len(flat_list)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 21, 2019

Contributor

why this?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 22, 2019

Author Contributor

This is because in the old way, an Array with one item will have a _loop_size equal to 0. I have opened an issue about it: #17230. Maybe there is a reason for keeping the _loop_size as 0? If so, I would be happy to know it. :-)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

If shape == (), maybe loop size should be 1? (i.e. it's a scalar)

index = [i for i in index]
for i in range(len(index), self.rank()):
index.append(slice(None))
index = tuple(index)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 21, 2019

Contributor

Can you put all this block in one line?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 22, 2019

Author Contributor

Yes :-)

if index >= self._loop_size:
raise ValueError("index out of range")
return index
# if isinstance(index, (SYMPY_INTS, Integer)):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 21, 2019

Contributor

Maybe you still need to handle this case. What about raising an exception?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 22, 2019

Author Contributor

Yes, I agree with you. We need to avoid passing an Integer to _parse_index, only the tuple index should be accepted. Raising an exception is more informative.

>>> a[0]
0
>>> a[2]
2

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 21, 2019

Contributor

Print the new output here.

@@ -48,6 +45,11 @@ def __getitem__(self, index):
if syindex is not None:
return syindex

if isinstance(index, Integer):
index = Tuple(index)
if not isinstance(index, (tuple, slice)):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 21, 2019

Contributor

why this?

@@ -48,6 +45,11 @@ def __getitem__(self, index):
if syindex is not None:
return syindex

if isinstance(index, Integer):
index = Tuple(index)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 21, 2019

Contributor

why not tuple?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 22, 2019

Author Contributor

I was trying to cast an Integer to a tuple: 1 -> (1, ), but the problem is that the builtin tuple doesn't accept only one Integer because it is not iterable. However SymPy's Tuple can handle this case.
An alternative would be: (index, ). Which one do you think is better? I was using Tuple because I think it can directly sympify the value.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

Go for index = (index,).

@@ -200,7 +200,7 @@ def test_ndim_array_converting():
assert(isinstance(matrix, SparseMatrix))

for i in range(len(sparse_array)):
assert sparse_array[i] == matrix[i]
assert sparse_array[sparse_array._get_tuple_index(i)] == matrix[i]

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 21, 2019

Contributor

maybe we need new tests for this feature.

- Ameliorated codes
- Added tests for new features of __getitem__()
@@ -164,7 +179,7 @@ def _new(cls, iterable, shape, **kwargs):
self._shape = shape
self._array = list(flat_list)
self._rank = len(shape)
self._loop_size = functools.reduce(lambda x,y: x*y, shape) if shape else 0
self._loop_size = functools.reduce(lambda x,y: x*y, shape) if shape else len(flat_list)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

If shape == (), maybe loop size should be 1? (i.e. it's a scalar)

@@ -48,6 +45,11 @@ def __getitem__(self, index):
if syindex is not None:
return syindex

if isinstance(index, Integer):
index = Tuple(index)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

Go for index = (index,).

array = ArrayType(range(24)).reshape(2, 3, 4)
assert array.tolist() == [[[0, 1, 2, 3], [4, 5, 6, 7], [8, 9, 10, 11]], [[12, 13, 14, 15], [16, 17, 18, 19], [20, 21, 22, 23]]]
assert array[0] == ArrayType([[0, 1, 2, 3], [4, 5, 6, 7], [8, 9, 10, 11]])
assert array[0, 0].tolist() == [0, 1, 2, 3]

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 23, 2019

Contributor

why not array comparison?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 24, 2019

Author Contributor

If shape == (), maybe loop size should be 1?

With the existing code, the loop size will be 1 if shape == (). I was using len(flat_list) rather than 1 because if we allow an empty array in the future, the ._array will be empty so that the loop size should be 0 not the default 1. In this case, len(flat_list) can show what the true length of array is.

why not array comparison?

Do you want to use ArrayType([0, 1, 2, 3]) instead of [0, 1, 2, 3]? I was trying to varify the test. Using array comparison is totally ok.

- Changed the way to convert integer to tuple
- Changed tests
assert array[0, 0] == ArrayType([0, 1, 2, 3])
value = 0
for i in range(2):
for j in range(3):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jul 25, 2019

Contributor

maybe add some subtests?

@Upabjojr Upabjojr merged commit 3295b02 into sympy:master Jul 25, 2019
3 checks passed
3 checks passed
codecov/project 74.574% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
@kangzhiq kangzhiq deleted the kangzhiq:new_iter branch Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.