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

[Don't merge][GSoC]Added ArrayComprehension class #16845

Merged
merged 12 commits into from May 28, 2019

Implemented functions and added tests

- Implemented _expand_array with iteration
- Implemented _check_bounds_validity
- Added doctest
- Added test in test_array_comprehension
  • Loading branch information...
kangzhiq committed May 22, 2019
commit 8effbb6600b7e4cfd7d7f5765dce7ba60ed680dc
@@ -4,12 +4,31 @@
from sympy.core.expr import Expr
from sympy.core import Basic
from sympy.core.compatibility import Iterable
from sympy.tensor.array import MutableDenseNDimArray
from sympy.tensor.array import MutableDenseNDimArray, ImmutableDenseNDimArray
from sympy import Symbol
from sympy.core.sympify import sympify
from sympy.core.compatibility import Iterable
from sympy.core.numbers import Integer

class ArrayComprehension(Basic):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 24, 2019

Contributor

This class is missing the .shape property, and the .rank() function as well. It should be compatible with the arrays.

"""
Generate a list comprehension
This conversation was marked as resolved by Upabjojr

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 22, 2019

Contributor

Can you add some example with doctests?

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq May 22, 2019

Author Contributor

I didn't know well itertools very well. I will have a look at it!
doctests will be added imediately.

If there is a symbolic dimension, for example, say [i for i in range(1, N)] where
N is a Symbol, then the expression will not be expanded to an array. Otherwise,
calling the doit() function will launch the expansion.
Examples
========
>>> from sympy.tensor.array import ArrayComprehension
>>> from sympy.abc import i, j, k
>>> a = ArrayComprehension(10*i+j, (i, 1, 4), (j, 1, 3))
This conversation was marked as resolved by Upabjojr

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

show that a has not been evaluated.

>>> a.doit()
[[11, 12, 13], [21, 22, 23], [31, 32, 33], [41, 42, 43]]
>>> b = ArrayComprehension(10*i+j, (i, 1, 4), (j, 1, k))
>>> b.doit()
ArrayComprehension(10*i + j, (i, 1, 4), (j, 1, k))
"""
def __new__(cls, expr, *bounds, **assumptions):
if any(len(l) != 3 or None for l in bounds):
@@ -18,16 +37,23 @@ def __new__(cls, expr, *bounds, **assumptions):
cls.default_assumptions = assumptions

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 24, 2019

Contributor

let this be handled by the superclass constructor.

obj = Expr.__new__(cls, **assumptions)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

the arguments expr and bounds need to be passed to the superclass in the canonical form. That is, you should be able to rebuild the same object with the arguments you pass to the superclass.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq May 23, 2019

Author Contributor

@Upabjojr Thank you for reviewing!
Now I understand better the structure of a new class.

obj.expr = expr

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

obj._expr = expr, then add a property for expr.

if cls._check_bounds_validity(bounds):
obj.bounds = bounds
obj.bounds = cls._check_bounds_validity(expr, bounds)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

same as above

arglist = [expr]
arglist.extend(obj.bounds)
obj._args = tuple(arglist)

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

WRONG. This is handled by the superclass in Expr.__new__, don't modify _args.

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq May 23, 2019

Author Contributor

@Upabjojr
Yes, I just modified the code. By the way, I was following the code of ExprWithLimits as an example, Code. So should these codes be standardized as well?

return obj

@classmethod
def _check_bounds_validity(cls, bounds):
return True
def _check_bounds_validity(cls, expr, bounds):
bounds = sympify(bounds)
for var, inf, sup in bounds:
if var not in expr.free_symbols:
raise ValueError('Varialbe {} does not exist in expression'.format(var))

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

why? what about generating constant arrays? I don't see any problem with ArrayComprehension(1, (i, 1, N)).

Also, add a test to check that this is possible.

if any(not isinstance(i, (Integer, Symbol)) for i in [inf, sup]):
raise TypeError('Bounds should be an Integer or a Symbol')

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

why? I don't see any problem with expressions as bounds. Maybe also add a test ArrayComprehension(..., (i, 0, N+1)).

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq May 23, 2019

Author Contributor

@Upabjojr Infact, I was trying to eliminate the cases where inf and sup are other than expressions, e.g. ArrayComprehension(k, (i, 1, [2, 3, 3, 3])) or ArrayComprehension(k, (i, 1, ArrayComprehension(k, (i, 1, 5)))), these don't really make sense.
I changed the condition to not isinstance(i, Expr)
But I haven't found a way then to check if bounds are Integer, e.g. ArrayComprehension(k, (i, 1, 3.55)), as the number will be sympified into Expr.

if isinstance(inf, Integer) and isinstance(sup, Integer) and inf > sup:
raise ValueError('Lower bound should be inferior to upper bound')
This conversation was marked as resolved by Upabjojr

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

if (inf > sup) == True:

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq May 23, 2019

Author Contributor

@Upabjojr Could you please tell me the advantage of doing so? Or it is a coding convention?

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 25, 2019

Contributor

you don't need to check that inf and sup are integers. The > sign creates a StrictGreaterThan object if it cannot determine the truth immediately. If you are sure the inequality is always true, it returns True.

Please remove isinstance(..., Integer).

This comment has been minimized.

Copy link
@kangzhiq

kangzhiq May 26, 2019

Author Contributor

Yes, you are right. In contrary, a bound with two expressions like (i,N+5, N+1) should not be valide. But it can still pass the test of inf > sup

return bounds

def doit(self):
expr = self.expr
@@ -39,8 +65,8 @@ def doit(self):
arr = self._expand_array()
return arr

# Add/replace a boudary of variable
def add_bound(self, bound):
# Substitute the variable with a value, so that the symbolic dimension can be expanded as well
def subs(self, var, val):
return 0

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

Remove this. This is handled by sympy/core and should not be overwritten.


This conversation was marked as resolved by Upabjojr

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 17, 2019

Contributor

maximum one newline inside indented namespaces.

def _expand_array(self):
@@ -52,18 +78,13 @@ def _array_subs(arr, var, val):
arr[index] = arr[index].subs(var, val)
return arr.tolist()

# Recursive function to perform subs at every variable according to its boundary
def f(expr, bounds):
if len(bounds)== 1:
var, inf, sup = bounds[0]
list_gen = [expr.subs(var, val) for val in range(inf, sup+1)]
return list_gen
else:
var, inf, sup = bounds[0]
list_gen = []
res = f(expr, bounds[1:])
for val in range(inf, sup+1):
list_gen.append(_array_subs(res, var, val))
return list_gen

return f(self.expr, self.bounds)
list_gen = self.expr
for var, inf, sup in reversed(self.bounds):
list_expr = list_gen
list_gen = []
for val in range(inf, sup+1):
if not isinstance(list_expr, Iterable):
list_gen.append(list_expr.subs(var, val))
else:
list_gen.append(_array_subs(list_expr, var, val))
return ImmutableDenseNDimArray(list_gen)
@@ -1,14 +1,23 @@
from sympy.tensor.array.array_comprehension import ArrayComprehension
from sympy.abc import i, j
from sympy.tensor.array import ImmutableDenseNDimArray
from sympy.abc import i, j, k, l
from sympy.utilities.pytest import raises


def test_array_comprehension():
a = ArrayComprehension(i*j, (i, 1, 3), (j, 2, 4))
b = ArrayComprehension(i, (i, 1, j))
c = ArrayComprehension(i*j, (i, 1, 3))
assert a.doit() == [[2, 3, 4], [4, 6, 8], [6, 9, 12]]
c = ArrayComprehension(i+j+k+l, (i, 1, 2), (j, 1, 3), (k, 1, 4), (l, 1, 5))
assert a.doit().tolist() == [[2, 3, 4], [4, 6, 8], [6, 9, 12]]
assert isinstance(b.doit(), ArrayComprehension)
assert b.subs(j, 3) == [1, 2, 3]
assert c.doit == [j, 2*j, 3*j]
c.add_bound((j, 2, 4))
assert c.doit() == [[2, 3, 4], [4, 6, 8], [6, 9, 12]]
assert isinstance(a.doit(), ImmutableDenseNDimArray)
#assert b.subs(j, 3) == [3, 6, 9]

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

You can still test subs.

assert c.doit().tolist() == [[[[4, 5, 6, 7, 8], [5, 6, 7, 8, 9], [6, 7, 8, 9, 10], [7, 8, 9, 10, 11]],
[[5, 6, 7, 8, 9], [6, 7, 8, 9, 10], [7, 8, 9, 10, 11], [8, 9, 10, 11, 12]],
[[6, 7, 8, 9, 10], [7, 8, 9, 10, 11], [8, 9, 10, 11, 12], [9, 10, 11, 12, 13]]],
[[[5, 6, 7, 8, 9], [6, 7, 8, 9, 10], [7, 8, 9, 10, 11], [8, 9, 10, 11, 12]],
[[6, 7, 8, 9, 10], [7, 8, 9, 10, 11], [8, 9, 10, 11, 12], [9, 10, 11, 12, 13]],
[[7, 8, 9, 10, 11], [8, 9, 10, 11, 12], [9, 10, 11, 12, 13], [10, 11, 12, 13, 14]]]]
raises(TypeError, lambda:ArrayComprehension(i*j, (i, 1, 3), (j, 2, [1, 3, 2])))

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr May 23, 2019

Contributor

space after lambda:

raises(ValueError, lambda:ArrayComprehension(i*j, (i, 1, 3), (j, 2, 1)))
raises(ValueError, lambda:ArrayComprehension(i*j, (i, 1, 3), (l, 1, 2)))
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.