Comment on revision r7aaa9ac803185e24bf0a20dd4969ba9a4dbca4e0 in pysph

[py...@googlecode.com]

Comment by prabhu.r...@gmail.com:

Score: Negative

General Comment:
I don't like the _tmp magically added and only once.

Line-by-line comments:

File: /source/pysph/sph/sph_calc.pyx
(r7aaa9ac803185e24bf0a20dd4969ba9a4dbca4e0)
===============================================================================

Line 220: # Add temporary array in case of fewer arguments to sph()
-------------------------------------------------------------------------------
This is always added? That seems unnecessary? If this is a problem them
it maybe makes sense to always have tmp{x,y,z} as props.
Line 244: output3 = self.dest.get_carray('_tmp')
-------------------------------------------------------------------------------
This is mighty fishy for if I supply three fewer args they will all be
written to _tmp which is plain wrong!

For more information:
http://code.google.com/p/pysph/source/detail?r=7aaa9ac803185e24bf0a20dd4969ba9a4dbca4e0

[py...@googlecode.com]

Comment by pankaj86:

General Comment:
_tmp is not meant to be used. It is only there as a placeholder to avoid
segfault for funcs which may write to it. If you do not supply any arg to
sph() then you shouldn't expect to get any usable value from it.
This is only because not all funcs implement num_outputs based on
dimensions.

Ex: in case of vector force with constant force along 'x', even in 1d
problem it will still write to three output arrays, which it shouldn't. So
where should it write the y and z components. Ideally it wouldn't write 3
outputs, but since its not written that way and i dont care for the y and z
outputs, it'll write to _tmp just so that it shouldn't segfault writing to
None. Not using three temporaries seems wasteful so there's only one, since
no one cares what their values are.

If you insist, i'll add all three temps, but note that it is wasteful and
not needed, and my goal is to get rid of the last one also by making the
remaining vector functions write only to the desired outputs and not all.

[py...@googlecode.com]

Comment by prabhu.r...@gmail.com:

Score: Positive

General Comment:
OK, I see your point but instead of writing to something that is thrown
away, I'd much rather the developer/user see an error immediately that the
number of arguments is wrong. I'd much rather see an exception thrown than
a warning message be written to a log file which I am not sure the user
will really read. :) I think that would be a better approach. We should
then enforce that num_outputs is always defined for a function, the
src/dest reads should also be defined and we insist that the right number
of arguments are supplied. Can that be done or am I missing something?

[Kunal Puri]

hi, sorry I was tied up over the weekend. The tmp properties provided a nice way to call a calc object without any explicit arguments, especially for testing and demonstration purposes. You could always bypass it by passing it explicit arguments. Moreover, the functions were a lot simpler IMHO, without the need for dimension checking as is being done currently in position_funcs. Maybe this is a case of unnecessary optimisation? I wouldn't want to go back to writing multiple functions/calcs based on dimension .


kunal

[Kunal Puri]

the tests are broken with the new head

[Pankaj Pandey]

Sorry for the mess over over the weekend, you can revert it back to changeset 389:ce31341554. I wont mess with it now, especially since i haven't been able to get opencl stuff working for testing.

[Kunal Puri]

On Mon, May 9, 2011 at 10:08 AM, Pankaj Pandey <pank...@gmail.com> wrote:

Sorry for the mess over over the weekend, you can revert it back to changeset 389:ce31341554. I wont mess with it now, especially since i haven't been able to get opencl stuff working for testing 

most of the tests relied on the tmp properties so you'd have to be careful if you remove them. I'm all for going back to 389 :)

[Prabhu Ramachandran]

True, but isn't it cleaner than keeping three additional properties all
the time?

regards,
prabhu

[Kunal Puri]

True, but only if were running close to resource limits which is quite generous on our target platforms.  No additional overhead for MPI as they are not being pickled and the functions are a lot cleaner that way.

Kunal