PR: Feature lattice prim cell

[kemattil]

Define Bravais lattices using a primitive cell.

Main features (files primitive_cell.py, abc_lattice_new.py, and lattice_new.py):

• a new data structure for representing primitive cells of a lattice (class PrimitiveCell)
• ABCLattice api changed, i.e. properties type and base_vect replaced with prim_cell
• standalone class Lattice takes a reference to PrimitiveCell at the initialization
• get_coordinates implemented in ABCLattice; uses the primitive vectors from PrimitiveCell (properties p1, p2, and p3)
• the factory functions make_XXX_lattice call the PrimitiveCell class methods create_cell_XXX_lattice
• the H5Lattice api remains almost intact, i.e. class method create_new should take prim_cell instead of type and base_vect, and the primitive cell should be read and stored in I/O

[tuopuu]

@kemattil, looks good. It's a surprisingly simple implementation, which is a good thing. (... but it doesn't have quite as many features as the competing one, however.) I'll give my vote to this.

One comment for the code already written: we should not use C-style format for floating point numbers: just 0 or 2, instead of 0.0 and 2.0.

If other guys from the dev-team prefer this proposal, I can take over and start writing tests, h5lattice, etc.

[itziakos]

There is no need to repeat the word cell in the methods

[itziakos]

Looking at the methods I would suggest to rename the classmethods just with the name of the cell e.g:

def cubic(cls, a)
def body_centered_cubic(cls)   # we should avoid cutting the words short

This will make the functions short and easier to understand

cell = PrimitiveCell.cubic(10)

[tuopuu]

I would get rid of the lattice word instead of cell.

edt. @itziakos's last post: that's even better.

[itziakos]

I think we can remove cell also. The info is in the class name

[itziakos]

perfect

[kemattil]

We have to be careful with the class method names: the methods create primitive cells for specific bravais lattices.

For example the specified primitive cell for a body-centered cubic lattice can itself be classified as a rhombohedral cell (i.e. it belongs to the rhombohedral symmetry group).

Therefore, a line cell = PrimitiveCell.body_centered_cubic(10) might be misleading ...

A line cell = PrimitiveCell.body_centered_cubic_lattice(10) appears more precise.
Note that the argument 10 specifies a (conventional) unit cell edge length, i.e.
the argument specifies a body-centered cubic lattice for which a primitive cell is created.

Would this be an acceptable compromise?

[itziakos]

The main problem is that these functions do not return a lattice.

Is there a unique name for this kind of lattice.
Do the other body_centered_cubic types need only one argument?

For the sake of user experience and a small cost in complexity we can support multiple signatures for body_centered_cubic so that more that one type of PrimaryCells can be returned.

[kemattil]

I'm not sure that I understand the questions, but the body_centered_cubic describes
a lattice type, i.e. there are no other body_centered_cubic types.

But there are infinitely many body_centered_cubic lattices and, moreover,
for any given lattice of this type, the primitive cell is not unique.

For example, the current implementation PrimitiveCell.body_centered_cubic
returns a specific primitive cell for a specific body-centered cubic lattice.

To allow construction of arbitrary primitive cells for arbitrary lattices of
a given type is a huge undertaking (I don't know how to do it).

On the other hand, the current initialization (i.e. __init__)provides the most general way of specifying primitive cells. However, at the initialization, the lattice type is designated
as triclinic by default (all other lattice types are special cases of triclinic lattices).

Currently the initialization does not allow users to specify the lattice type, because
we do not know how to check whether the provided primitive vectors really
define a lattice of the given type (a very non-trivial task) ...

[kemattil]

Is cell = PrimitiveCell.for_body_centered_cubic_lattice(10) better?

[itziakos]

So far it looks very good and concise. Had only minor comments. Looking forward for the complete PR

[kemattil]

@tuopuu , I have made the small changes proposed last week (consensus on the PrimitiveCell class method names or signature of PrimitiveCell.__init__ has not yet been achieved).

Can you continue from here? Note that the code is absolutely not tested ...

[tuopuu]

Can you continue from here? Note that the code is absolutely not tested ...

@kemattil I'll continue from here.

[tuopuu]

I have updated the tests for primitive vector representation of the Lattice and H5Lattice classes. Old H5Lattice implementation was only modified slightly, so that we still use Pytables attributes of the table structure to store size,origin and primitive vectors of the lattice. Because of the file backend changes, I have changed the LATTICE_CUDS_VERSION to 2.

Please note, that there is no conversion tool for H5CUDS files available for this change: we need to decide where to put these tools in the SimPhoNy file structure (maybe simphony/utils?). (Here, writing the conversion script is trivial, however.)

Please also note, that documentation is not up to date, and that Appveyor build fails. I don't know why. It should have nothing to do with this branch...

@kemattil, @roigcarlo, @nathanfranklin, this branch is ready for review. Please take a look.

[nathanfranklin]

In order to avoid using the non-public _bravais_lattice attribute here, I would suggest possibly reworking PrimitiveCell so that bravais_lattice is also a parameter in PrimitiveCell's __init__

[tuopuu]

Good idea. PrimitiveCell can be modified to include bravais_lattice parameter. It will also simplify some of the classmethods.

[kemattil]

@tuopuu, I think this PR if fine.

[nathanfranklin]

👍

[tuopuu]

Added tests for PrimitiveCell class method parameters so that proper error are returned when the values are not on the allowed range. Also removed the test for cell volume greater than zero in the PrimitiveCell constructor, because that test always passes when edge length and edge parallelity tests pass.

I think this PR is ready to be merged, but waiting for one +1 for the final additions.

[kemattil]

+1