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.

Sign up for GitHub

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

Detect and remove duplicate dihedrals with the same periodicity #530

Merged
merged 7 commits into from
Nov 27, 2024
Merged

Detect and remove duplicate dihedrals with the same periodicity #530

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
124ff98
Fix grammar in comment
LSchwiebert Aug 7, 2024
6b2a8ea
Remove duplicate dihedrals with the same periodicity and issue a warning
LSchwiebert Aug 7, 2024
02bfcf6
Generate error instead of warning if duplicates have different parame…
LSchwiebert Aug 8, 2024
61d0398
Also check for duplicate Bonds, Angles, etc. and minor code cleanup
LSchwiebert Aug 21, 2024
34c80ab
Enhance patch to print filename in warnings
LSchwiebert Aug 23, 2024
0dd9bcd
Update new warning messages
LSchwiebert Aug 24, 2024
a3a7c1c
Update warning message for duplicated dihedrals
LSchwiebert Aug 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/FFSetup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,14 +380,26 @@ void Dihedral::Read(Reader &param, std::string const &firstVar) {
if (index == 0) {
// set phase shift for n=0 to 90 degree
// We will have C0 = Kchi (1 + cos(0 * phi + 90)) = Kchi
//this avoids double counting the C0 (constant offset) term, which is used force fields like TraPPE
// this avoids double counting the C0 (constant offset) term, which is used
// in force fields like TraPPE
def = 90.00;
}
Add(merged, coeff, index, def);
last = merged;
}
void Dihedral::Add(std::string const &merged, const double coeff,
const uint index, const double def) {
// Check for (and skip) duplicate periodicities for the same dihedral
bool duplicate = false;
for (auto it = n[merged].begin(); it != n[merged].end(); ++it) {
duplicate |= *it == index;
}

if (duplicate) {
std::cout << "Warning: Skipping duplicate periodicity of " << index
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch works as intended per my testing.

I looked at what NAMD does. It appears they take the last one input. We may want to be consistent with them for py-mcmd.

My last comment, is more of a question, and this would go for non-bonded and all bonded parameters.

Do we want to more than NAMD and check if the parameters are the same or different (this would be more than NAMD):

  • if the same, skip it
  • if different, throw an error or overwrite with the last one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested is and it Looks like it is working as intended.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the patch so that it throws an error if the parameters differ by more than 0.001. I have to imagine that would be a mistake. Since it only proceeds if they match, then which one we save doesn't matter.

So, we do the same thing as NAMD if the parameters match except we also issue a warning, and we terminate with an error if the parameters differ.

<< " for dihedral " << merged << "!\n";
return;
}
++countTerms;
Kchi[merged].push_back(EnConvIfCHARMM(coeff));
n[merged].push_back(index);
Expand Down