Specifiable options for periodic neighbors calculations #318
Conversation
…ding max neighbors count
Signed-off-by: Lee, Kin Long Kelvin <[email protected]>
laserkelvin
added
bug
…cture creation Signed-off-by: Lee, Kin Long Kelvin <[email protected]>
pymatgen should now internally at least shift to unit cell based on documentation
| for site in dst_sites: | ||
| if site_count > max_neighbors: |
There was a problem hiding this comment.
are neighbors sorted in any way? if not it might make sense to prune the lists after the loop.
There was a problem hiding this comment.
I haven't fully verified it, but under the hood for ase.neighborlist, the code uses scipy.spatial.cKDTree which should be sorted based on nearest neighbors: i.e. I think if it's not sorted for the whole list of indices, it should be at least for the same level of the tree.
In other words with a max_neighbors of 50+ you're definitely going to be getting the nearest neighbors, but towards the end of the 50, those neighbors might not be necessarily sorted.
| gamma = gamma * np.pi / 180.0 | ||
|
|
||
| # this matrix is normally for fractional to cart | ||
| rotation = torch.tensor( |
There was a problem hiding this comment.
maybe add a link to the wiki where this came from, for future reference.
There was a problem hiding this comment.
Added in 973a1b9
This PR closes #317 by allowing optional keyword arguments to the
PeriodicPropertiesTransformworkflow:max_neighborsprovides a hard cap on the destination nodes for any atom. A counter is updated within the per-site loop, and will break out if we exceed themax_neighborsvalue.allow_self_loops, when set toFalse(which is now the default), will filter out self-loops as the last step, just before we return the updated data dictionary.I also caught and fixed two bugs in the
aseimplementation:adapative_cutoffcellreshape before passing it into theeinsum