peterbjorgensen / DeepDFT

Official implementation of DeepDFT model
MIT License
56 stars 8 forks source link

PBCs not accounted for in neighbor lists? #3

Closed BurnerJ closed 1 year ago

BurnerJ commented 1 year ago

Hi there, when playing around with the code, I noticed that when I load a cube using the _read_cube function and check atoms.get_pbc(), I get [False False False]. Never is this set to True in the rest of the code, so it seems the resulting connectivity table does not account for PBCs (specifically in the CollateFuncAtoms and CollateFuncRandSample classes). Does this have to do with some specific cube formatting in the ase.io.cube class? Is it supposed to recognize pbc's automatically? I'm wondering if PBCs should automatically be turned on in these custom collatefn classes. I suspect so... since the source code for the ase io.cube class only sets atoms.pbc = True if the argument program == "castep" (which is not the case for enabled PBCs in this code).

Thanks for any clarification here!

Edit: I suppose it is handled properly by asap3 after looking through the docs (and thus the wrapper code you wrote is only use for the cases with no PBCs). Please disregard if that is the case!

peterbjorgensen commented 1 year ago

Hi, thanks for commenting on this. I think the problem is that there is no standard way of saving the pbc in cube files. I am just relying on ASE to do the right thing, but I think in the current stable version of ASE cube files will always come up as pbc=False unless the cube files are detected as castep files. I can see that they have changed this in the git version, but that means (as far as I can tell) that the cube files will always be periodic if you use the git version of ASE.

I could add a flag to force PBC on if that would help.

BurnerJ commented 1 year ago

Thank you for the quick response! I will just force PBCs for my purposes.