search

ComputationalRadiationPhysics / picongpu

Performance-Portable Particle-in-Cell Simulations for the Exascale Era :sparkles:
https://picongpu.readthedocs.io
Other
693 stars 218 forks source link

Make Vector.shrink not shift contents if srcDim==dstDim #1173

Open Flamefire opened 8 years ago

Flamefire commented 8 years ago

The Vector.shrink method can be used to make a n-i dimensional vector out of a n dimensional vector. However if one e.g. wants to have always a 2D vector one would expect that vec2D.shrink<2>(1) would not alter the vector. But the values get shifted in every case.

I propose adding a check if srcDim==dstDim and returning the vector unchanged in that case.

ax3l commented 8 years ago

I can't follow the full behaviour you are describing, can you add an example pls? Are you talking about that shrink?

What would vec2D.shrink<2>(0) do? Causing a shift with startIdx=1 looks well behaved to me.

Flamefire commented 8 years ago

Yes this is the function i meant.

What I mean is this method is called shrink. So you expect that to shrink the vector to the given number of dimensions. Having this cause a shift is at least completely unexpected. Especially in templated code you want to make sure, you have a vector of a given number of dimensions (e.g. a 2D vector out of a 2D or 3D simdim-vector) so you call shrink on it. Causing a rotation in a 2D simulation is vertainly not what you intended.

ax3l commented 8 years ago

you expect that to shrink the vector to the given number of dimensions. Having this cause a shift is at least completely unexpected.

yes that is an unideal mix of functionality, but the problem is: each shrink looses information and people want to choose which information (component) shall be lost :)

Isn't vec2D.shrink<2>(0) what you want? Your proposal would actually introduce a special case to "ignore the offset parameter" in case the dimension stays the same, which makes it imho even more complicated.

Flamefire commented 8 years ago

I still think that shrink<n> means 'make this vector have n dims'. But yes it ignores the parameter as the full meaning (currently) of shrink<n>(i) is 'make this vector have n values from this vector starting at index i'. So probably shrink is an unfortunate name then.

ax3l commented 8 years ago

yes thought so too because it basically selects components from an offset round-robin style and creates a new (possibly shrinked, newDim <= oldDim) vector