-
Notifications
You must be signed in to change notification settings - Fork 59
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
[RFC] More band-aid fixes to Colvars #700
base: reusable-cvcs
Are you sure you want to change the base?
Conversation
After some thoughts I feel there is no way to make explicit gradients working correctly with reusable components, so I will disable it in this PR. |
This does not work as Colvars was not designed with automatic differentiation in mind.
The problem of
|
AST Giacomo said that variables_active_smp is used for making the colvar object appearing in the loop multiple times for the original parallelization scheme. As I understand, this means that I can use variables_active directly to build the AST instead of the duplicated items in variables_active_smp.
if (it_find == cvc_info_map.end()) { | ||
cvc_info_map.insert({parent, cvc_info{ | ||
// TODO: Here calc_cvc_values calls cvcs[i]->is_enabled() , which is is_enabled(int f = f_cv_active) | ||
// I know both f_cv_active and f_cvc_active are 0 but are they the same option?? |
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 fact that f_cv_active
and f_cvc_active
have the same numerical value has no consequence, they should never be used in the same context, because they are respectively only meaningful in a colvar
or cvc
object. The colvardeps
data of these two classes are non-overlapping. The relationship between them is a vertical (parent-child) dependency.
If we were to merge the two levels, then these two features would merge.
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 issue is that in
Line 1437 in 0f2d682
if (!cvcs[i]->is_enabled()) continue; |
is_enabled()
is called, and since there is no function parameter passed, so I think it would call Line 167 in 0f2d682
inline bool is_enabled(int f = f_cv_active) const { |
which checks
f_cv_active
instead of f_cvc_active
. My code is to follow what the original calls in calc_cvc_values
and I am not sure if I need to follow it the same way.
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.
You're right, sorry. That is somewhat sloppy writing that I didn't remember well. It does rely on the "active" property being number 0.
Line 224 in 0f2d682
// NOTE that all feature enums should start with f_*_active |
But a class inheriting from colvardeps
could also override is_enabled()
and change this convention.
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.
How much effort do you think it would be to remove the default argument for this virtual function? Remember that this is one of the "issues" that clang-tidy
was complaining about.
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.
That would be very little effort. I'm happy to do that if that helps in any way.
it != cv->variables_active_smp()->end(); ++it) { | ||
// TODO: Bad design! What will happen if CVC a is in a "colvar" block | ||
// that does not support total_force_calc, but is then reused in | ||
// another block that requires total_force_calc even if it supports Jacobian itself??? |
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 don't see the problem here. Assuming a cvc can have several parents: the colvar that does require a total force can enable it in its children cvcs, even if other parents don't require (or support) 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.
You are right. I just thought that if a colvar does not require the total force, then it would disable the corresponding feature of the children, but it seems the code do not check the dependency in that way.
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.
It's the other way: disabled by default, and enabled on request - then disabled again if the refcount falls to zero.
lambda_fn, NULL, CKLOOP_NONE, NULL); | ||
} | ||
cvm::decrease_depth(); | ||
return cvm::get_error(); |
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.
Parallelization and checking the error
When I wrote and test new code, I found that NAMD may not exit cleanly (some other threads were still running) if one thread was exited. I think the implementation of colvarproxy_namd::error
or at least its use in the parallel region, needs to be revised to use a std::atomic<bool>
to setup an error "bit" instead of calling NAMD_die
directly. Then after all threads are finished and joined, we can use cvm::get_error()
to check the error bit and exit if there is an error.
There seems to be a misunderstanding here. To clarify, the spirit of |
Thanks for the clarifications. You are right that colvars/src/colvarcomp_combination.cpp Lines 58 to 60 in ff35f9c
Also, I cannot use add_child in colvardeps to declare that a sub-CVC is a child of the other, as add_child also does dependency checking, so I think that it is a bad design to check dependencies while calling the initialization function. Calling the init means that Colvars is still parsing options, and the abstract syntax tree is not completely built, but the dependencies are semantic things that should be done after the AST is completely built.
In my opinion, the following structure should be separated from Lines 155 to 162 in ff35f9c
to form the AST, and colvardeps should only be a feature-dependency checker, and not own the AST. Adding new children to the AST should not trigger the checker. In other words, it is better that colvardeps acts somewhat like an LLVM pass.
|
Thanks @HanatoK , I now understand your point better and I fully agree! To move the AST out of colvardeps and allow same-level dependencies, we need to deal with the fact that children objects of a CVC can be CVCs or atom groups, so those should be described by different parts of the feature tree. |
@HanatoK - this is a large and disruptive change set. I think there are items in there that we can agree on. Having an AST alongside the colvardeps class is one. Your initial point 2 was: Bypass the colvar class and take the cvc objects out to build the AST. I think that the colvar class should be completely removed; |
Well, I believe that no one would really like this PR. This PR uses a lot of hacks and workarounds in the backend to achieve reusable CVCs in SMP with limitations as less as possible (although I am not a big fan of distributing CVCs over threads), including:
colvardeps
looks like an AST but after playing with it I feel it is not a real one, and it really just checks the dependencies of features, and there is no true AST. It would be better if Colvars could be redesigned with a true AST and a dependency checker for it. The dependency checker should not own the AST;colvar
class and take thecvc
objects out to build the AST. I think that thecolvar
class should be completely removed;smp_lock
andsmp_unlock
incolvarproxy_namd
are implemented as creating and destroying locks, so I have changed them;colvar::cvc::modify_children_cvcs_atom_gradients
andpropagate_colvar_force
). When callingcalc_gradients
andapply_force
of a CVC consisting of sub-CVCs, it now propagates the gradients and forces to all its sub-CVCs;smp_lock
. However, it is very coarse-grained so I expect an additional performance penalty. I thought there should be a lock tied to each atom group but found none.In summary, I think that Colvars should be fundamentally changed to achieve better support of reusable components and parallelization.
This PR tries to solve #232, extends #644 and finishes: