Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Hbond update #3848
Hbond update #3848
Changes from 9 commits
c47513d
0140005
340cd3f
d1a7182
f7f8bf5
299b898
bd95900
5d27895
7903c4f
7af74d8
9966f74
77af51e
4ae3b7b
a61b56e
333ef6b
049c0ee
2788935
a607605
50dd832
8cf40a1
d5fbe48
0a16b13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only quickly going through this so I might be missing something obvious - doesn't this assume that all types are the same? Wouldn't this be a problem if you end up with a type where some of the atoms are outside the bounds of the allowable charges but not others?
My recommendation here would be to default to a unique attribute, like the
ix
value, instead of using something that's broad and could pick up stuff outside the intended atomgroup selection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IAlibay In most cases an atom type in LAMMPS is all identical with the exception of forcefields such as ReaxFF and COMB. Although you are right that LAMMPS allows for atoms to have individual charges so this could be an issue as forcefields develop...
Here's the issue, these structures look for a list of atom selection strings that represent categories, so if this was on an index basis, the list would be very long, and the resulting output would be useless (representing a single interaction). One could have this structure instead search the atoms of each type and make a long string that specifies the specific indices that meet the charge criteria. However, this string will be unnecessary computational time in 99% of cases, and in the 1% I would argue it would skew the results as these many-body forcefields will continue to fluctuate in charge throughout the simulation, as they are supposed to. I would argue counting "low" or "high" charges in that case wouldn't disrupt the intention of the calculation, it would result in a broken hydrogen bond with a more "realistic" decay time due to local atom environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think we should have the option to choose whether or not to default to types here, however for the sake of getting this done I'm going to let it go as-is here, and raise a separate issue to deal with later.