JuliaMolSim / DFTK.jl

Density-functional toolkit
https://docs.dftk.org
MIT License
440 stars 89 forks source link

Datastructure for `(lattice, atoms)` #308

Open antoine-levitt opened 4 years ago

antoine-levitt commented 4 years ago

We do [model.recip_lattice * G for G in G_vectors(basis)] quite a lot; it's annoying and error prone (I've several times had the bug that I used lattice instead of recip_lattice. Should we have a G_vectors_cart? The problem is that if we want to do that for kpoints we have to have a backref to basis in kpoints - should we just have that?

mfherbst commented 4 years ago

I disagree about the backref. I know we had it in the original design and later killed it for some reason, so I don't think reeintroducing it will be any good. I don't see the problem of having a G_vectors(basis, :cartesian) version, though.

mfherbst commented 4 years ago

Or of also storing the cartesian version of the G-vectors in the k-points

antoine-levitt commented 4 years ago

I think I had another problem where I wanted a backref in the kpoints. If you don't want a backref to basis, maybe we can have a backref to model at least? Otherwise we're just going to keep adding back fields whenever we need them

mfherbst commented 4 years ago

Model feels like the better solution too me even though ideally I would only want it to have the part that defines the lattice and the atoms, because a single k-point does not depend on the terms or the spin or the symmetry for example.

antoine-levitt commented 4 years ago

Compartmentalization like that is just more trouble than it's worth: it gives you a fuzzy feeling of Good Software Practices but does not actually lead to any actual code decoupling. OK I'll just add a backref to model.

mfherbst commented 4 years ago

Actually in this case it does. We use the combination of atoms and lattice a lot and I think it makes sense to add some nice support functions for this tuple. Essentially that is exactly that "structure"-type object you want here as well.

antoine-levitt commented 4 years ago

Oh that I agree