Integration with ASE? - Githubissues

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

Integration with ASE? #3

Closed RKBK closed 7 years ago

RKBK commented 7 years ago

It seems reasonable that the kgrid function be integrated in the ASE package itself. It would likely be pretty useful also for many other people. Would there by any reason not to do this?

ajjackson commented 7 years ago

This is an appealing idea. At the moment, the various calculators of ASE have different interfaces for specifying k-point grids. Some of these include one-parameter options, and the two one-parameter specifications built into VASP are currently being added to its calculator, for example. It will take a little ingenuity to come up with an acceptable implementation which allows the users of various calculators to proceed as expected while also making a general convenience function available.

RKBK commented 7 years ago

You're quite right that it would be nice if you could put it in directly into the calculators in general. However, I also think there could be much value in just implementing it "as it is", and having it as a convenience function somewhere in ASE. Then, the way it would be used would be for the user to

import kgrid (or whatever it will be called) from ASE
define the system
calculate the k-point sampling for the system using kgrid
introduce that k-point sampling into the calculator object when defining it. (This means that the calculator will do it's own reduction of the number of k-points in the irreducible Brillouin zone. I don't know if that's a problem or not.)

ajjackson commented 7 years ago

In e4ea0b6 I have re-factored kgrid so that from Python we can pass in an ase.atoms object and receive a tuple. This tuple is generally accepted by ASE calculators for specifying the k-point mesh. I think this gives a fairly clean interface to ASE and have documented it in the README. While it's not in ASE, this is just how the convenience function would behave if it were part of ASE; the only difference at this stage is that they are not being distributed together! As we are making other changes to the code at this point, I think this is a good compromise. Once the code has stabilised, maybe then we can petition the ASE developers to take a look at it!

RKBK commented 7 years ago

Your changes look great. I think it will be easy to get it into ASE, changes and improvements are always welcome.

ajjackson commented 7 years ago

I'm inclined to agree, and have so far found the ASE community to be active and welcoming. I'll close this issue for now as I think the key goal of easy integration has been achieved. We can open a new issue "contribution to ASE" once we are more confident with the implementation (i.e. having addressed Issue #4 !)