removal of tmpx.. properties

[Pankaj Pandey]

The recent change to move tmpx.. to _tmpx.. has caused a problem with parallel runs since the load balancer does not create _tmpx.. properties for transferred arrays as it is not pickled. Hence to fix this issues i think of the following ways:

1. Make _tmpx.. special and exist for all ParticleArrays
2. Do away with _tmp properties. Since SPHFunctions already know their num_outputs, the calc can verify that a correct number of arguments are called to the SPHCalc.sph() - no default arguments. So all output_array arguments have to be explicit and not beginning with '_'

I'm leaning towards (2) as it seems much cleaner to me (explicit is better than implicit). Comments/suggestions are welcome.

[Prabhu Ramachandran]

Hi,

On Saturday 07 May 2011 02:03 AM, Pankaj Pandey wrote:
> The recent change to move tmpx.. to _tmpx.. has caused a problem with
> parallel runs since the load balancer does not create _tmpx.. properties
> for transferred arrays as it is not pickled. Hence to fix this issues i
> think of the following ways:
>
> 1. Make _tmpx.. special and exist for all ParticleArrays
> 2. Do away with _tmp properties. Since SPHFunctions already know their
> num_outputs, the calc can verify that a correct number of arguments are
> called to the SPHCalc.sph() - no default arguments. So all output_array
> arguments have to be explicit and not beginning with '_'

Can you elaborate on 2? I thought tmpx etc. were simply temporary
arrays that were created to store temporary calculations. In this case
it makes little sense to actually send the array. Instead we should
ensure that on unpickling the temporary properties exist. So maybe on
pickle/unpickle we need to save some arrays and for the rest just save
their name and datatype and recreate on unpickle.

cheers,
prabhu

[Pankaj Pandey]

The tmp arrays were for temporaries, but the integrator does not use tmpx.., instead it uses the '_' arrays.
So the only place tmpx.. is used is in SPHCalc.sph() if it is called with default arguments and not provided with the names of the particle arrays to output to.
The integrator calls sph() with specific arguments not tmpx. Also i dont think there is any good reason to let calcs evaluate to tmp by default. It should be upto the user (or indirectly integrator) to use know where its output should go. If it is needed to be output to tmpx then the user must ensure that tmpx exist, and nothing should

I've a local patch which removes the tmp arrays, and also fix the num_outputs of the SPHFuncs to appropriate number. If num_outputs is <3 then the other output arrays to SPHFunc.eval() are None as called from SPHCalc.sph(). The calc also ensures that the updates list of its constructor is of appropriate length.
This will also enable us to completely do away with setting all 3 arrays to 0 even if num_outputs < 3
My basic point was that tmps are hardly being used anywhere, so it makes sense to simply remove them resulting in 3 less arrays.

I'll post my patch soon for review to get a more clear idea about it.

Pankaj

[Pankaj Pandey]

Here's the patch to remove tmp properties. Some more changes are needed to make position funcs honor the updates variable instead of updating all of x,y,z, but thats the basic idea.

Pankaj

[Prabhu Ramachandran]

On Saturday 07 May 2011 06:16 PM, Pankaj Pandey wrote:
> The tmp arrays were for temporaries, but the integrator does not use
> tmpx.., instead it uses the '_' arrays.
> So the only place tmpx.. is used is in SPHCalc.sph() if it is called
> with default arguments and not provided with the names of the particle
> arrays to output to.
> The integrator calls sph() with specific arguments not tmpx. Also i dont
> think there is any good reason to let calcs evaluate to tmp by default.
> It should be upto the user (or indirectly integrator) to use know where
> its output should go. If it is needed to be output to tmpx then the user
> must ensure that tmpx exist, and nothing should
>
> I've a local patch which removes the tmp arrays, and also fix the
> num_outputs of the SPHFuncs to appropriate number. If num_outputs is <3
> then the other output arrays to SPHFunc.eval() are None as called from
> SPHCalc.sph(). The calc also ensures that the updates list of its
> constructor is of appropriate length.
> This will also enable us to completely do away with setting all 3 arrays
> to 0 even if num_outputs < 3
> My basic point was that tmps are hardly being used anywhere, so it makes
> sense to simply remove them resulting in 3 less arrays.
>
> I'll post my patch soon for review to get a more clear idea about it.

OK, so that sounds good, I think you can either check it in and notify
us so we can review the patch or maybe just post the patch so we can
discuss.

cheers,
prabhu

[Prabhu Ramachandran]

Looks good to me but I would defer to Kunal's opinion.

cheers,
Prabhu