Comment on revision r55b9f4d1c63355457550e785d663fe58d826f0b7 in pysph
py...@googlecode.com
Score: Positive
General Comment:
I love the fact that you are testing the funcs as you add the OpenCL code.
This would ensure that any results will be trustworthy! There are a few
design issues about the test that I would like to discuss and the comments
inline should help start the discussion.
Line-by-line comments:
File: /source/pysph/sph/funcs/energy_funcs.clt
(r55b9f4d1c63355457550e785d663fe58d826f0b7)
===============================================================================
Line 5: /* Thermal energy equation (eq 10) described in "A shock-captiting
SPH
-------------------------------------------------------------------------------
Typo in shock-capitating. :)
File: /source/pysph/sph/tests/test_energy_funcs.py
(r55b9f4d1c63355457550e785d663fe58d826f0b7)
===============================================================================
Line 84: def setup_cl(self):
-------------------------------------------------------------------------------
It looks like this entire setup process is similar in all the function
tests. I think it might be best to abstract it out so this doesn't have to
be repeated every time. Maybe an SPHFunctionTest would be better?
The test also suggests that it isn't easy to do the cl stuff since you are
essentially re-doing what the calc already does? Doesn't the calc already
have the code to actually create the program and set everything up?
Instead this test only ensures, among other things, that
solver.create_program is correct (and the calc actually uses another code
path different from solver.create_program). This is sub-optimal. Ideally,
the way to test it would be to just create a calc, evaluate the cython
solution (which is done nicely with the func.eval) and also the cl stuff in
a similar fashion.
Having one single test to recreate the func without the calc is good but
testing everything this raw way seems to me a the incorrect way to
proceed. Testing the calcs seems like a much better way to test and
ensures that the calcs (which is what the user will use) work correctly.
We can discuss this if you feel otherwise.
For more information:
http://code.google.com/p/pysph/source/detail?r=55b9f4d1c63355457550e785d663fe58d826f0b7
py...@googlecode.com
Score: Positive
General Comment:
Forgot to mention, notice the code in bench/cl_summation_density.py, that
code is so much cleaner when it uses the calcs rather than all the setup
work you have to do in these tests.