-
Notifications
You must be signed in to change notification settings - Fork 5
WIP branch for implementing ABCLattice change. #191
Conversation
@kemattil, @itziakos, @roigcarlo, @nathanfranklin This proposal is now open for discussion. Please, take a look when you got time. |
@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) ... |
self._primitive_vectors = (p1, p2, p3) | ||
self._type = 13#TRICLINIC_3D #default behaviour | ||
|
||
def create_cubic_3d(self, size, origin, a): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternative constructors are commonly defined as classmethods
@classmethod
def create_cubic_3d(cls, size, origin, a):
"""
Creats a cubic lattice
"""
primitive_vectors = calculate_primitive_vectors(a, a, a, 90.0, 90.0, 90.0)
return cls(size, origin, primitive_vectors, kind=LATTICEKIND.CUBIC_3d)
And there is no need for create_new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't make using classmethod to work with all the differences between H5Lattice, Lattice (and proxylattice). H5Lattice has the group parameter which Lattice doesn't have, and also the way of generating the table for data is different in these both. Proxylattice has it's own specific features with the reference pointer to external data.
Therefore, distinct functionality has to be stored in methods inside a child class of Abstractlattice, and these methods have to be somehow called from Abtractlattice "alternative" constructors such as create_cubic_3d. I don't see a way to do it if classmethod is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
H5Lattice is to be initialized only inside the H5CUDS file please see comment (#191 (comment))
It does not make sense for the user to be allowed to create HDF5 lattice backed files on an arbitrary group. Do you have a usecase where this is necessary?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
H5Lattice is to be initialized only inside the H5CUDS file please see comment
You're right. We don't have a usecase to use other groups. This is a good point that can potentially simplify things a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, for lattice-boltzmann HPC applications, we should be able to construct hdf5-backed lattice without generating the whole lattice in memory first, because often the lattice is much bigger than can be fit into a single PC memory. This doesn't require the use of 'group' parameter, but we should still allow generation of hdf5 backed lattice somehow, because currently it's not supported inside H5CUDS (that only has get.../add... methods for CUDS objects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is a valid usecase, which we should address, (by either by providing a create_dataset
or my preferred option using preferably the 'sparse' dict based allocation), but not by mangling the initialization of python objects :P
why not |
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:
Creating an H5Lattice on its own is not something that we should provide. It would be better if we provide a |
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 4d40c31). This proposal will result in small/moderate changes to existing code and to a simple class design/hierarchy. Main features (files
The issue of handling very large lattice can be discussed separately. |
@kemattil, please, make a WIP pull request so we can start discussing your proposal. |
WIP pull request made, #205 |
Primitive vector representation will be implemented in PR #205. I'll close this one. |
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:
only sets the name of the lattice
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.
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.
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.