search

WMD-group / kgrid

Calculate the required k-point density from the input geometry for periodic quantum chemistry calculations
Other
20 stars 12 forks source link

Reciprocal lattice vectors #4

Closed ajjackson closed 7 years ago

ajjackson commented 7 years ago

Currently kgrid naively uses the reciprocals of the lattice vectors, rather than the reciprocal lattice vectors. The schemes should be fairly equivalent for orthorhombic cells, but moving to a truly reciprocal lattice-based scheme may give better performance for non-orthorhombic cells and is probably what Moreno and Soler intended.

If this is implemented, how should the compatibility break be handled? The scheme could be explicitly requested with a flag but this would discourage it's use. We could just tick up a major version number and break compatibility...

RKBK commented 7 years ago

I'm in favor of getting it right without worrying too much about compatibility. Certainly, (I assume) you'd end up with slightly different grids before and after, but results obtained with grids from kgrid both before and after should still be OK, right? Since kgrid claims, "on the tin", to implement the method of Moreno and Soler, and since they did some thorough work to support their method, it seems good to try to conform to it.

ajjackson commented 7 years ago

To be clear, Moreno and Soler's paper largely addresses optimisations that are beyond the scope of this code as they require more control and knowledge of the final symmetry-reduced mesh. This code implements a way of finding mesh parameters which is consistent with the length cutoff definition in their paper - Moreno and Soler go further and show ways of reaching that cutoff optimally. In any case, I agree that "getting it right" is a high priority. It should be possible to get high-quality results with either approach as long as convergence is tested properly, but I don't want to catch anyone out who is half-way through a study. I'd better start a proper changelog.

The other reason I haven't already implemented the change (which is itself fairly trivial) is that I would like to also carry out some testing and experimentation to understand the implications better, and that increases the time requirement. Still, the Issue is now open and it should happen at some point! I would be interested to hear from people like @aronwalsh @keeeto @JMSkelton who have experience with the "naive" approach about their experience of how reliable it is and where it breaks down.

ajjackson commented 7 years ago

This has now been implemented in commit 2623a32 and seems to give the same results in orthorhombic cases and different results for other shapes, as expected. Backwards compatibility is broken, but the old behaviour can still be used with the -r flag at the command line or with realspace=True in the Python interface.