Comment on revision rce31341554349067e28292ea331f8c80cf51691b in pysph
py...@googlecode.com
Score: Positive
General Comment:
Overall it looks good but I am not sure if the opencl support is guaranteed
to work also it isn't clear as to where and why the tmp* arrays are used,
and do the standard examples work with this changeset? The bug mentioned
below should also be fixed.
Line-by-line comments:
File: /source/pysph/sph/funcs/arithmetic_funcs.pyx
(rce31341554349067e28292ea331f8c80cf51691b)
===============================================================================
Line 40: prop_names=['rho'], constant=0.0, **kwargs):
-------------------------------------------------------------------------------
Isn't this a bug? Since self.prop_names = prop_names below, if you modify
the arguments every instance will see the same exact prop_names and it will
modify future calls to the class. Take for instance the following code:
class A(object):
def __init__(self, x=['a']):
self.x = x
def add_prop(self, val):
self.x.append(val)
a = A()
a.add_prop('b')
b = A()
b.x
You will see that b.x is ['a', 'b']! Which is not what we want. So we
should never set default args to mutable quantities. Could you please fix
this in the entire code I suspect there are other places. The solution is
to set prop_names=None and special case that.
File: /source/pysph/sph/sph_func.pyx
(rce31341554349067e28292ea331f8c80cf51691b)
===============================================================================
Line 333: pass
-------------------------------------------------------------------------------
This should raise an error IMHO since the point is that a developer should
be warned early that the src/dest args are not overridden. If not the user
might run into all kinds of nasty problems. Essentially, the developer of
a new SPHFunction should be forced to implement this. What do you think?
File: /source/pysph/sph/tests/test_boundary_funcs.py
(rce31341554349067e28292ea331f8c80cf51691b)
===============================================================================
Line 58: cs=cs, tmpx=mf,
tmpy=mf, tmpz=mf,
-------------------------------------------------------------------------------
The only problem I have with this approach is that over here, it isn't
clear at all as to why tmp* arrays are needed at all?
File: /source/pysph/sph/tests/test_update_smoothing.py
(rce31341554349067e28292ea331f8c80cf51691b)
===============================================================================
Line 231: for integrator in [solver.EulerIntegrator]:
-------------------------------------------------------------------------------
Why aren't the other integrators tested or are they not fixed yet?
For more information:
http://code.google.com/p/pysph/source/detail?r=ce31341554349067e28292ea331f8c80cf51691b