simphony / simphony-common

The native implementation of the Simphony cuds objects
BSD 2-Clause "Simplified" License
7 stars 5 forks source link

WIP branch for implementing ABCLattice change. #191

Closed tuopuu closed 9 years ago

tuopuu commented 9 years ago

Lattices with primitive vector representation

The main reason to moving from base vector representation to primitive vectors is that it allows defining the type of the lattice more conclusively. In a typical use-case, the user would call new ABCLattice methods that replace old factory functions to create a specific lattice. There are some points that I would like to make, that give some reasoning to the implementation proposed here:

Firstly, calculation of primitive vectors based on lattice constants a,b,c and the angles between vectors alpha, beta, gamma would have to be done separately for all supported lattice types in their respective factory functions. However, there is a formula (the triclinic one) that can be used for generating the primitive vectors for all lattice types, with a catch, that it will be hard to make (float) comparisons of the primitive vectors of different lattices. Typically, in LB-codes lattice nodes are compared with respect to their indices, so that inaccuracy e.g. get_coordinates is not very relevant. I think that using the same formula for every lattice type is a good compromise because it significantly reduces the amount of code. Calculation of primitive vectors can be added inside ABCLattice as a concrete method, so that it is accessible from all child classes later.

Secondly, constructors of Mesh and Particles classes currently generate empty container, but Lattice ctor generates a full lattice, based on the given size,origin,etc parameters. To that end, Lattice (and H5Lattice with its create_new classmethod) needs a big list of parameters, contrary to Mesh and Particles ctors that only need one, the name. I suggest the following: XLattice ctor only takes as parameters the ones that are specific to the implementation. For examples, Lattice always needs a name, but H5Lattice only needs a group, and not a name. Separating specific functionality to XLattice ctor allows implementing many shared functions of the lattices in ABCLattice as concrete methods and XLattice would inherit all that from there. I think that technically they would have to be regular def create_xxx_3d(self, ...) style methods so that they would work correctly when called from within a child class, such as H5Lattice. Including create_xxx_3d methods with ABCLattice has a benefit that it would provide factory method functionality to the other lattice classes as well, instead of only the Lattice. I propose that lattice classes would work the following way:

A = Lattice('somename') 

only sets the name of the lattice

B = Lattice('somename').create_cubic_3d( (10,20,30), (0,0,0), a)

first creates the lattice base class with name 'somename'. ABCLattice.create_cubic_3d calls ABCLattice constructor, which sets the parameters a,b,c,etc. accordingly, calls ABCLattice._set_primitive_vectors, and finally calls Lattice._create_new which finally generates the lattice table.

C = H5Lattice(group)

Check if group already has a lattice stored in it. If yes, then generate H5Lattice from the stored information. If no, then just store group to self._group.

C = H5Lattice(group).create_cubic_3d( (10,20,30), (0,0,0), a)

ABCLattice.create_cubic_3d calls ABCLattice ctor with correct parameters, sets primitive vectors, and finally calls H5Lattice._create_new which writes a,b,c,etc attributes to the file/group pointed by group, and also generates Pytables arrays for storing data.

tuopuu commented 9 years ago

@kemattil, @itziakos, @roigcarlo, @nathanfranklin This proposal is now open for discussion. Please, take a look when you got time.

kemattil commented 9 years ago

@tuopuu, I defnitely would like to review and comment on this. I'm currently (trying) to have my vacation. I hope this can be kept open until I return to office (around mid-August) ...

itziakos commented 9 years ago

B = Lattice('somename').create_cubic_3d( (10,20,30), (0,0,0), a)

why not Lattice.cubic_3d(name, parameters)

itziakos commented 9 years ago

Secondly, constructors of Mesh and Particles classes currently generate empty container, but Lattice ctor generates a full lattice

This is not necessary to happen like this, it is the current native Lattice implementation that requires a full Lattice not the api. One can always:

C = H5Lattice(group)

Creating an H5Lattice on its own is not something that we should provide. It would be better if we provide a create_dataset method on the H5CUDS file and in the various engines. Or create an empty Lattice as proposed earlier. In sort the hdf5 cuds container objects are not to be used initialized outside the H5CUDS file object

tuopuu commented 9 years ago

Thank you, @itziakos, for the comments. I'm not sure if I understand correctly how your way of implementing primitive vectors would work for all lattices, but I'll give it a try.

We also had a long discussion with @kemattil about two weeks ago, and he also had good ideas about how to change this.

kemattil commented 9 years ago

I think there are now so many aspects/issues interlinked that we need to simplify things a little bit.

Therefore I made a new proposal for implementing all 3D Bravais lattices (see the branch feature-lattice-prim-cell 4d40c31854ee3292e3a905158ed8334cc9cda7cf). This proposal will result in small/moderate changes to existing code and to a simple class design/hierarchy.

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

The issue of handling very large lattice can be discussed separately.

tuopuu commented 9 years ago

@kemattil, please, make a WIP pull request so we can start discussing your proposal.

kemattil commented 9 years ago

WIP pull request made, #205

tuopuu commented 9 years ago

Primitive vector representation will be implemented in PR #205. I'll close this one.