-
Notifications
You must be signed in to change notification settings - Fork 61
[WIP] Extended dynamics for harmonic restraints #874
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks @fhh2626 ! I don't have time for an in-depth review right now, but as a reminder to myself I'll write here some points I'd like to watch for:
|
src/colvar.h
Outdated
| /// \brief Link this colvar's components to any required biases | ||
| int link_biases(colvarmodule *cvm); | ||
|
|
||
| /// \brief Get a pointer to the i-th component (CVC) | ||
| cvc* get_cvc_ptr(size_t index); | ||
| cvc const* get_cvc_ptr(size_t index) const; // Const version too | ||
|
|
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 think it should be possible to avoid these intrusive changes to the colvar class, if the linkage is driven by the bias instead. That is, at init time, the bias could be given a colvar to link to, look it up by name (that is already implemented) and set the mutual pointers.
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.
Thanks for your comment. I've removed the link_biases function and done the reverse lookup, but I am not sure how to bypass get_cvc_ptr.
src/colvarbias_restraint.cpp
Outdated
| // Loop over all associated collective variables | ||
| for (size_t i = 0; i < num_variables(); i++) { |
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.
Unless I'm misunderstanding this, the force constant update should be done once, not once per colvar, so it should not be inside this loop.
More generally, this reimplements colvarbias_restraint::update() with a lot of duplication. Instead, this should be implemented as a variant of the moving restraint code: this is another way of updating the force constant. It would be very clear if the update was done in a similar way, calling the linked cv instead of computing the new force constant internally.
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 have reused the current implementation of update().
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.
Please have a look whether this version addresses your comment.
a58f906 to
86d35fd
Compare
|
Whenever possible we rebase branches onto master rather than merging master. I've done that here. |
| // update restraint energy and forces | ||
| error_code |= colvarbias_restraint::update(); | ||
| // The base class update() zeros out energy and forces, so it must be called first. | ||
| colvarbias::update(); |
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.
Maybe I was not clear in my first review. Since this is a moving force constant scheme, I think it should integrate into colvarbias_restraint_k_moving and the update can be done in the well-named function update_k().
| for (size_t i = 0; i < cvm->num_variables(); ++i) { | ||
| colvar* cv = (*(cvm->variables()))[i]; | ||
| for (size_t j = 0; j < cv->num_cvcs(); ++j) { | ||
| cvc_harmonicforceconstant* hfc_cvc = dynamic_cast<cvc_harmonicforceconstant*>(cv->get_cvc_ptr(j)); |
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.
What I meant before is, the name of the CV to reference could be provided as a new parameter in the bias configuration, so no reverse lookup would be needed, just a regular lookup by CV name.
I recommend a slightly different separation of tasks between the CV (and its constituent CVC) and the bias.
Since the CV's value is lambda, it does not need to know about the exponent n. The bias needs to know about n, so it should compute the force on lambda for extended Lagrangian integration and apply it to the CV using apply_bias. Effectively, this bias acts on the restrained CVs, but also on the lambda CV, because its energy depends on CV values as well as lambda.
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.
Thanks Jerome! So what does your expected Colvars config file look like? Could you please provide an example to prevent misunderstanding? I can revise the code to adapt it.
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.
The user interface could be something like:
colvar {
name lambda_cv
extendedLagrangian on
extendedMass 1500000
extendedLangevinDamping 5000
reflectinglowerboundary on
reflectingupperboundary on
harmonicForceConstant # Similar to alchLambda, no parameters needed
}
harmonic {
colvars d
forceConstant 100. # k_max
dynamicForceConstantLambda lambda_cv
dynamicForceConstantExponent 4.0
}
When initializing the harmonic restraint, it can look up the cv by name and do the linking, that is, set a pointer to itself in the CVC. The CVC would complain if asked to calculate itself when the bias pointer is not set.
As mentioned in #649 , this PR makes it possible to use a harmonic restraint as a collective variable (CV). Enhanced sampling algorithms can then be used to calculate the free-energy change associated with creating or removing the harmonic restraint(s).
Examples:
Next steps: