make_xxx_lattice on a plane functions ignore the z coordinates of origin.

[itziakos]

The z coordinates of the origin argument are ignored by:

• make_square_lattice
• make_rectangular_lattice
• make_hexagonal_lattice

[tuopuu]

@itziakos, this is not a bug, but instead an intended behavior. It's a matter of definition: How do we treat a 2D lattice that is embedded in 3D? It has 2 relevant dimensions, but on the other hand, dimension 1 is defined for it because of the fact that we embed lattices in 3D in SimPhoNy. It makes the 2D lattice a special case of a 3D lattice.

For a 2D lattice, it does not make sense to define 3rd coordinate, unless we start speaking of a 3D lattice (with dim 1 to one of the primitive vector dimensions) that is an embedded version of the corresponding 2D lattice.

Could you, please, be more specific what would be the interface for 2D lattices you'd like to have?

ps. I'd be happy to change origin to 2D for the 2D lattices.

[itziakos]

I think that there is some confusion here, I am not suggesting an interface change. I am talking only about the specific factory functions in the issue description.

The current implementations of the factory functions create a 2D lattices embedded on the XY plane in 3D. Currently the implementation will ignore the z coordinates of the origin attribute and always place the XY plane were the lattice is defined at the plane were z==0 (see https://github.com/simphony/simphony-common/blob/master/simphony/cuds/lattice.py#L199). Thus If I want to create a 'Square' lattice at the plane were z==1.0, I cannot use the factory function. Yet, one can achieve the z==1.0 using the Lattice constructor which supports setting z==1 for 2D lattices.

So based on the fact that make_square_lattice does not allow me to create a lattice where z==1.0 even though the Lattice object supports that behavior to me is a (arguably small) bug. If this is the intended behavior of the make_square_lattice then is it bug because:

• The documentation is not mentioning this behavior.
• The signature of the factory function accepts a three x,y,z tuple for origin.

[tuopuu]

You're right, there are some inconsistencies in the documentation and implementation. It was intended, that for 2D factory functions 3rd coordinate would be ignored and the lattice would always lay on the XY-plane. I could change the origin to 2D for 2D factory functions, which may be the quickest/cleanest solution. We may eventually have to rethink this because of the interface change from base vectors to primitive vectors. Would this quick fix be OK for now?

Anyway, I fully agree with you that Z != 0 would be useful, and equally useful would be allowing the embedding of 2D lattices on any plane from XY,XZ, or YZ. All this would require changes to factory functions, for example, adding of tests that the size vector has '1' as one of it's elements.

[itziakos]

You're right, there are some inconsistencies in the documentation and implementation. It was intended, that for 2D factory functions 3rd coordinate would be ignored and the lattice would always lay on the XY-plane. I could change the origin to 2D for 2D factory functions, which may be the quickest/cleanest solution. We may eventually have to rethink this because of the interface change from base vectors to primitive vectors. Would this quick fix be OK for now?

This will be a step backwards especially because it changes the signature of the factory functions. Why not update the factory functions to pass origin[3] to the origin attribute of the Lattice constuctor?
This should be a simpler change and easy to test

For example make_square_lattice:

 return Lattice(name, 'Square', (h, h, 0), tuple(size)+(1,), (origin[0], origin[1])+(0,))

Will become:

 return Lattice(name, 'Square', (h, h, 0), tuple(size)+(1,), origin)

As far as I know no other changes will need to take place and the documentation will be consistent.

Now, regarding the when this bug needs to be fixed is another question which we can discuss on the next dev-meeting (not every bug needs to be fixed immediately, but it is important to document them). At the end we might even decide to wait until the Lattice description changes.

Anyway, I fully agree with you that Z != 0 would be useful, and equally useful would be allowing the embedding of 2D lattices on any plane from XY,XZ, or YZ. All this would require changes to factory functions, for example, adding of tests that the size vector has '1' as one of it's elements.

We do not have to support XZ and YZ at this stage. However, as shown above the changes are simple to support XY planes. It is also going to be very easy to test.

[tuopuu]

Now, regarding the when this bug needs to be fixed is another question which we can discuss on the next dev-meeting (Not every bug needs to be fixed immediately, but it is important to document them)

Fair enough, we can discuss this on Monday.

[tuopuu]

Fixed in #205