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

New __iter__ for Array module #17308

Merged
merged 14 commits into from Aug 11, 2019

Conversation

@kangzhiq
Copy link
Contributor

commented Jul 31, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

The array iteration is modified to have a new behavior:
Before:

>>> a = Array([[1, 2], [3, 4]])
>>> for i in a:
...     print(i)
...
1
2
3
4
 

Now:

>>> a = Array([[1, 2], [3, 4]])
>>> for i in a:
...     print(i)
...
[1, 2]
[3, 4]

Besides, the iteration over each item is moved to a new object named nditer :

>>> from sympy.tensor.array.nditer import nditer
>>> for i in nditer(a):
...     print(i)
...
1
2
3
2
3
4

Other comments

Release Notes

  • tensor
    • Modified iter function and added new class named nditer
kangzhiq added 2 commits Jul 31, 2019
New Iteration for Array
- Modified iteration function inside Array
- Added nditer object to iterate every element
- Modified _getitem_ to fix the case of A[:]
- Modified code and tests accordingly
@sympy-bot

This comment has been minimized.

Copy link

commented Jul 31, 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 iter function and added new class named nditer (#17308 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
The array iteration is modified to have a new behavior:
Before:
```python
>>> a = Array([[1, 2], [3, 4]])
>>> for i in a:
...     print(i)
...
1
2
3
4

```
Now:
```python
>>> a = Array([[1, 2], [3, 4]])
>>> for i in a:
...     print(i)
...
[1, 2]
[3, 4]
```
Besides, the iteration over each item is moved to a new object named `nditer` :
```python
>>> from sympy.tensor.array.nditer import nditer
>>> for i in nditer(a):
...     print(i)
...
1
2
3
2
3
4
```

#### 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 __iter__ function and added new class named nditer
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.


try:
if isinstance(self._iter, list):
result = self._iter[self._idx]

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 31, 2019

Author Contributor

How to dynamically handle the iteration over a multi-dimension list is a great challenge. Especially for the case where an array is cast to list:

>>> a = Array([[1, 2], [3, 4]])
>>> b = list(a)
>>> b.__class__
<class 'list'>
>>> b[0].__class__
<class 'sympy.tensor.array.dense_ndim_array.ImmutableDenseNDimArray'>

I might need some guidance in this problem. :-)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

Please ignore list. We have Array in SymPy. People who use list will use 99% of the time NumPy, we don't need support for list.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

We don't need to redo what NumPy already does... if you want to use NumPy's features, just use NumPy.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 2, 2019

Author Contributor

But we have tests like:

 assert list(sparse_array) == [0, 0, 0, 1]

That is why I ran into an error about list and tried to handle this special case. However, I have changed the test by using a simple tolist() instead of 'list()'.

assert list(-5 * BA(i1, i2)) == [-5 * i for i in list(ba_matrix)]
#assert list(BA(i1, i2)) == list(ba_matrix)
#assert list(3 * BA(i1, i2)) == [3 * i for i in list(ba_matrix)]
#assert list(-5 * BA(i1, i2)) == [-5 * i for i in list(ba_matrix)]

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Jul 31, 2019

Author Contributor

New tests might be needed.

@Abdullahjavednesar Abdullahjavednesar requested a review from smichr Aug 1, 2019

from sympy.tensor.array.ndim_array import NDimArray
from sympy.core.compatibility import Iterable

class nditer(object):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

SymPy standard is NDIter

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

This should be a SymPy object, so inherit Expr.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

Maybe this object should be called Flatten, as what it does it flattening the array.


if not isinstance(iterable, (Iterable, MatrixBase)):
raise NotImplementedError("Data type not yet supported")
if isinstance(iterable, list) and isinstance(iterable[0], NDimArray):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

why this condition?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 2, 2019

Author Contributor

This condition is to handle to case where an array is cast to list by list():

>>> a = list(Array([[0, 1], [2, 3]]))
>>> a.__class__
<class 'list'>
>>> a[0].__class__
<class 'sympy.tensor.array.dense_ndim_array.ImmutableDenseNDimArray'>

And the multdimensional list is not considered here, as you explained below. I was thinking the same so I only handled this special case of having a mutipledimensional list from list().

from sympy.tensor.array.ndim_array import NDimArray
from sympy.core.compatibility import Iterable

class nditer(object):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

Maybe this object should be called Flatten, as what it does it flattening the array.

@@ -0,0 +1,58 @@
from sympy.tensor.array.dense_ndim_array import DenseNDimArray

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

please no new files, there is a file for array operations.

self._idx = 0

def __iter__(self):
return self

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

why not return the iterator here?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 2, 2019

Author Contributor

I was thinking the NDIter itself as an iterator, that is why self is returned here.


try:
if isinstance(self._iter, list):
result = self._iter[self._idx]

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

Please ignore list. We have Array in SymPy. People who use list will use 99% of the time NumPy, we don't need support for list.


try:
if isinstance(self._iter, list):
result = self._iter[self._idx]

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

We don't need to redo what NumPy already does... if you want to use NumPy's features, just use NumPy.

def __next__(self):
from sympy.matrices.matrices import MatrixBase

try:

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 1, 2019

Contributor

No try please.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 2, 2019

Author Contributor

I was using try to handle the IndexError so that the iterator can be stopped properly by raising a StopIteration. I have seen the usage of try+except structure in SymPy's codebase. Should it be avoided?

Renamed class to Flatten
- Renamed class to Flatten
- Fixed a bug of index
@@ -296,3 +296,57 @@ def permutedims(expr, perm):
new_array[i] = expr[t]

return type(expr)(new_array, new_shape)


class Flatten(object):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 2, 2019

Contributor

inherit Basic


if not isinstance(iterable, (Iterable, MatrixBase)):
raise NotImplementedError("Data type not yet supported")
if isinstance(iterable, list) and len(iterable)>0 and isinstance(iterable[0], NDimArray):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 2, 2019

Contributor

if it's a list, then transform it into an Array.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 5, 2019

Author Contributor

If we transform it to Array, there will be a loop of calling functions:
While initiating an Array, we will call Flatten() in return.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 5, 2019

Contributor

You cannot add a list to a SymPy object. What about calling flatten( ) in Array?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 6, 2019

Author Contributor

OK!

@sylee957 sylee957 added the tensor label Aug 3, 2019

kangzhiq added 2 commits Aug 5, 2019
Test passed
Fixed error in diff()

if not isinstance(iterable, (Iterable, MatrixBase)):
raise NotImplementedError("Data type not yet supported")
if isinstance(iterable, list) and len(iterable)>0 and isinstance(iterable[0], NDimArray):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 5, 2019

Contributor

You cannot add a list to a SymPy object. What about calling flatten( ) in Array?

return self._array[index]
# if isinstance(index, slice):
# return self._array[index]
# else:

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 5, 2019

Contributor

if you are removing code, just remove it


shape, flat_list = cls._handle_ndarray_creation_inputs(iterable, shape, **kwargs)
shape = Tuple(*map(_sympify, shape))
cls._check_special_bounds(flat_list, shape)
flat_list = flatten(flat_list)
flat_list = flatten(Flatten(flat_list))

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 5, 2019

Contributor

is this really needed? isn't flatten already supposed to flatten the list?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 6, 2019

Author Contributor

No, it can be removed. I didn't think about it. Thank you!!
Since I have modified the flatten() function in the last commit, it can now flatten the result of list(Array()).
What we had before is that it would run into the case of having 'args' and transform an Array to Arrays.args, which will add the shape into the flatten list.

 if hasattr(el, 'args'):
    el = el.args
Updated code and added doctest and tests
- Updated code for simplification
- Added documentation
- Added tests for Flatten
_ Updated test_tensor
@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

There is a conflict with master. You need to merge the master branch and resolve the conflict.

kangzhiq added 3 commits Aug 7, 2019
@codecov

This comment has been minimized.

Copy link

commented Aug 8, 2019

Codecov Report

Merging #17308 into master will increase coverage by 18.656%.
The diff coverage is 87.5%.

@@              Coverage Diff               @@
##            master    #17308        +/-   ##
==============================================
+ Coverage   56.045%   74.702%   +18.656%     
==============================================
  Files          631       631                
  Lines       163078    163201       +123     
  Branches     38272     38286        +14     
==============================================
+ Hits         91398    121915     +30517     
+ Misses       65979     35951     -30028     
+ Partials      5701      5335       -366
def test_sympy__tensor__array__arrayop__Flatten():
from sympy.tensor.array.arrayop import Flatten
from sympy.tensor.array.dense_ndim_array import ImmutableDenseNDimArray
fla = Flatten(ImmutableDenseNDimArray(range(24)).reshape(2, 3, 4))

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 8, 2019

Contributor

where is the call to _test_args?

>>> A = Array(range(6)).reshape(2, 3)
>>> Flatten(A)
Flatten([[0, 1, 2], [3, 4, 5]])
>>> [*Flatten(A)]

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 8, 2019

Contributor

is this valid in Python 2.7?

def __next__(self):
from sympy.matrices.matrices import MatrixBase

try:

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 8, 2019

Contributor

please remove this try. You should define the total number of elements and then count up to them.

# Construction from another `NDimArray`:
elif isinstance(iterable, NDimArray):
shape = iterable.shape
iterable = list(iterable)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 8, 2019

Contributor

why was this removed???

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq Aug 9, 2019

Author Contributor

Because we are trying to cast an Array to list and then flatten it in the constructor with flatten(). Since flatten() can now flatten an Array properly, I was thinking that we don't need this case any more.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Aug 10, 2019

Contributor

OK

@Upabjojr Upabjojr merged commit 5ec4afc into sympy:master Aug 11, 2019

3 checks passed

codecov/project 74.702% (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_iteration branch Aug 12, 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.