Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix distance nb #91

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix distance nb #91

wants to merge 2 commits into from

Conversation

albapa
Copy link
Member

@albapa albapa commented Sep 17, 2024

fixed permuational invariance bug for order>2 distance_nb:

  1. order the atoms belonging to a cluster according their Zs, so that they appear in the same order as in the Z template
  2. multiply all pair-wise bond cutoffs together

@bernstei
Copy link
Collaborator

I had the impression you were going to do something more sophisticated than just multiplying all the pairwise cutoffs, but it looks to me like (line 10198 in the new version) it just naively multiplies all the pairwise cutoffs. Isn't that going to lead to contribution zero for (0, 0, 0), (rcut-Delta, 0, 0) (-rcut+Delta, 0, 0), when Delta is small, because of the 2-3 pair, while we actually want to have some non-zero contribution?

@albapa
Copy link
Member Author

albapa commented Sep 19, 2024

Yes, this is intentional - but maybe we want something different. By compact cluster I mean those which all atoms are connected - and this is now reflected in the implementation. On the other hand, compact_cluster=F includes the type of cluster you mentioned above and indeed does a bit more "sophisticated" cutoff.

By the way, the atom-centred angle_3b descriptor also includes this type of triplet.

@bernstei
Copy link
Collaborator

That's completely counter to my intuition for what the cutoff means. It's definitely not the way the cutoff is interpreted for conventional 3-body potentials like SW or Tersoff.

I agree that the range that's passed to LAMMPS for the ghost atom layer needs to be the value of the cutoff that's used in the all-pairwise product. But I'm wondering if we should double the cutoff (for Nb > 2) by default, just to be consistent with the way other potentials quote their "cutoff".

@albapa
Copy link
Member Author

albapa commented Sep 19, 2024

Yes, because they have a central atom reference which we try to eliminate here.

I don't mind using twice the cutoff - although I'd be worried about side-effects, such as the effective cutoff and neighbour list becoming larger than the user signed up for!

Just to be clear, I am not wedded to this idea at all. If there is another way to consolidate this descriptor, I don't mind.

@bernstei
Copy link
Collaborator

bernstei commented Sep 19, 2024

What about distance_Nb with compact_clusters=T enumerating the N-body groups based on a central atom and its Nb-1 neighbors up to the cutoff (like normal angle potentials do)? Isn't that the least unexpected?

[edited] that's also what ACE does
[more edits] just for enumerating - the actual assignment of permutations would still ignore which was the "central" atom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants