Fixing Arithmetic / Memory Complexity #840
Conversation
…all the tests passing
| @@ -50,11 +50,11 @@ def assemble_matrix_ex01(ne1: 'int', ne2: 'int', | |||
| v = 0.0 | |||
| for g1 in range(0, k1): | |||
| for g2 in range(0, k2): | |||
| bi_0 = basis_1[ie1, il_1, 0, g1] * basis_2[ie2, il_2, 0, g2] | |||
| # bi_0 = basis_1[ie1, il_1, 0, g1] * basis_2[ie2, il_2, 0, g2] | |||
There was a problem hiding this comment.
I think this has changed your computational intensity results. Do you not need bi_0/bj_0? Do you think it makes sense to change the expected intensity or are these here for a reason? We could just return them to avoid them being seen as useless?
There was a problem hiding this comment.
I think as long as it is just a function we are using for testing purpose it would be better to delete them and edit the complexity value
There was a problem hiding this comment.
Ok. Then should we delete them rather than just commenting?
| if self.mode: | ||
| return ops + 1 | ||
| else: | ||
| return ops + Symbol('ARCTANH') |
There was a problem hiding this comment.
I wonder if these can be simplified now that #754 has been fixed? I.e by doing:
def _cost_NumpyUfuncBase(self, expr, **settings):
ops = sum(self._cost(i, **settings) for i in expr.args)
if self.mode:
return ops + 1
else:
return ops + Symbol(expr.name.upper())
| @@ -3,6 +3,11 @@ | |||
| # This file is part of Pyccel which is released under MIT License. See the LICENSE file or # | |||
| # go to https://github.com/pyccel/pyccel/blob/master/LICENSE for full license details. # | |||
| #------------------------------------------------------------------------------------------# | |||
| # pylint: disable=R0201, missing-function-docstring | |||
|
|
|||
| # TODO: - Pow | |||
There was a problem hiding this comment.
_cost_PyccelPower is defined at line 234. Does it still need to be in the TODO?
| # TODO add other numpy array constructors | ||
| if isinstance(expr.rhs, (NumpyZeros, NumpyOnes)): |
There was a problem hiding this comment.
Can this TODO be handled by just doing if isinstance(expr.rhs, NumpyNewArray)?
| return sum(count_ops(i, visual) for i in expr) | ||
| else: | ||
| raise NotImplementedError('TODO count_ops for {}'.format(type(expr))) | ||
| return ntimes * ( self._cost(expr.rhs, **settings) ) |
There was a problem hiding this comment.
If the multiplication is only done in _cost_Assign then the wrong cost will be calculated for standalone expressions. Has this case been considered?
| def _cost_AugAssign(self, expr, **settings): | ||
| # TODO add other numpy array constructors | ||
| if isinstance(expr.rhs, (NumpyZeros, NumpyOnes, Comment, EmptyNode)): |
There was a problem hiding this comment.
See above can't you use NumpyNewArray?
Also why do you have Comment and EmptyNode? I don't think it is possible to do a += #None
|
|
||
| print('----------------------') | ||
| i = 0 | ||
| comp = [READ*n**3/2 + 3*READ*n**2/2 + READ + WRITE*n**3/6 + 3*WRITE*n**2/2 + WRITE*n/3 + WRITE, |
There was a problem hiding this comment.
See above, function name please
|
|
||
| print('----------------------') | ||
| i = 0 | ||
| comp = [2*READ + WRITE*k1*k2 + WRITE*(p1 + 1)*(p2 + 1) + 2*WRITE + ne1*(READ + WRITE + ne2*(READ + WRITE*k1*k2 + WRITE + (p1 + 1)**2*(p2 + 1)**2*(14*READ + 6*WRITE + k1*k2*(18*READ + 7*WRITE)) + (p1 + 1)*(p2 + 1)*(READ*(p1 + 1)*(p2 + 1) + WRITE) + (p1 + 1)*(p2 + 1)*(READ + WRITE + k1*(READ + WRITE + k2*(5*READ + 2*WRITE)))))] |
There was a problem hiding this comment.
See above, function name please. Also could you please break this line up so it fits on a screen please
| files = sorted(os.listdir(path_dir)) | ||
| files = [os.path.join(path_dir,f) for f in files if (f.endswith(".py"))] |
| files = sorted(os.listdir(path_dir)) | ||
| files = [os.path.join(path_dir,f) for f in files if (f.endswith(".py"))] |
| files = sorted(os.listdir(path_dir)) | ||
| files = [os.path.join(path_dir,f) for f in files if (f.endswith(".py"))] |
| end = ((e-b) / step)-1 | ||
| # translate so that we always start from 0 | ||
| return summation(ops, (i, 0, end)) |
There was a problem hiding this comment.
Translating so that the index always starts from 0 leads to wrong results if loops inside loops depend on the index. E.g:
def test(degree : int):
a = 0
for j in range(1, degree+1):
for r in range(j):
a += 1
return a
|
Unfortunately your PR is not passing the tests so it is not quite ready for review yet. Let me know when it is fixed with The failing tests are:
|
Fixing the computation of Arithmetic , Memory and Intensity Complexity