Collect atomic gradients for combination cvcs #247
Conversation
jhenin
commented
Apr 11, 2019
jhenin
commented
- delegate work of colvar::collect_cvc_gradients(), which was very intrusive, to cvc::get_gradients()
- overload virtual get_gradients() in CVCs such as dihedPC
- enables gradient display for dihedPC in VMD dashboard
- could be extended to other "special" CVCs
- in the process fix destructor bug that lead to segfault (double delete)
- delegate work of colvar::collect_cvc_gradients(), which was very intrusive, to cvc::get_gradients() - overload virtual get_gradients() in CVCs such as dihedPC - enables gradient display for dihedPC in VMD dashboard - could be extended to other "special" CVCs - in the process fix destructor bug that lead to segfault (double delete)
src/colvarcomp.h
Outdated
| @@ -163,6 +163,10 @@ class colvar::cvc | |||
| /// \brief Calculate finite-difference gradients alongside the analytical ones, for each Cartesian component | |||
| virtual void debug_gradients(); | |||
|
|
|||
| /// \brief Calculate atomic gradients and add them to the corresponding item in gradient vector | |||
| /// May be overridden by CVCs that do not store their gradients in the classic way, see dihedPC | |||
| virtual void get_gradients(std::vector<int> const &atom_ids, std::vector<cvm::rvector> &atomic_gradients); | |||
There was a problem hiding this comment.
Totally agree with the move to colvar::cvc.
The name get_gradients() just sounds a bit like a pure accessor. How about splitting it into collect_gradients() and get_gradients()?
There was a problem hiding this comment.
The thing is, there is no data structure to hold the gradients at the CVC level. I think that should be the case eventually, or something similar (as part of #61 ), but in the meantime I didn't see a solution to split these.
There was a problem hiding this comment.
That is true, but even if the gradients aren't stored yet in the CVC object, they can still be an argument to both functions.
There was a problem hiding this comment.
I mean, the results of the gradient calculation cannot be stored anywhere but in the atomic_gradients vector. If collect_gradients does that, then there is nothing left for an accessor to do. Actually, I think that is the case, so let me rename this to collect_gradients.
src/colvarcomp.h
Outdated
| @@ -1148,6 +1152,8 @@ class colvar::dihedPC | |||
| virtual ~dihedPC(); | |||
| void calc_value(); | |||
| void calc_gradients(); | |||
| /// Re-implementation of cvc::get_gradients() to carry over atomic gradients of sub-cvcs | |||
| void get_gradients(std::vector<int> const &atom_ids, std::vector<cvm::rvector> &atomic_gradients); | |||
There was a problem hiding this comment.
How about colvar::alpha_angles?
There was a problem hiding this comment.
Actually writing this raised the question of addressing this situation in a more generic way: a CVC that combines several sub-CVCs. I didn't move forward yet because I see a few obstacles to handling that in an elegant way, e.g.: (1) the precise mathematical combination will be specific to each combination CVC, and (2) the dependency system is not designed to handle this kind of "horizontal" dependency (CVC to another CVC), although that could probably be added.
There was a problem hiding this comment.
It seems a bit of work to derive a general template function for only two exceptions. With one already covered, it's mostly a matter of who deals with the other one.
There was a problem hiding this comment.
That matter is now settled :-)
In passing, remove bug-causing destructor ~h_bond()
| // fit gradients are in the unrotated (simulation) frame | ||
| atomic_gradients[a] += coeff * fg.fit_gradients[k]; | ||
| } | ||
| for (size_t k = 0; k < ag.size(); k++) { |
There was a problem hiding this comment.
Realized treating all the atom group rotation cases was unnecessary for these sub-CVCs!
|
Forgot to merge this, thanks. I updated the version strings in 9fab4ab |
