Repository navigation
Conversation
This commit refactors the read_data and read_data_gpu of atom groups. The atom groups may be referenced by different CVCs in the future.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical lifecycle and calculation-order bugs can dereference deleted objects, use stale coordinates and gradients, and apply stale forces from inactive components.
6 open issues
What changed in this PR
This PR moves atom-group preparation from individual components into the Colvars module, laying groundwork for group reuse. It also centralizes boundary-condition updates so the GPU path refreshes the lattice once per calculation.
Changes:
- Add a reference-counted atom-group registry and module-level preparation and force-processing loops.
- Move fitting-gradient, total-force, and gradient-debugging stages into module scheduling.
- Replace component-held boundary conditions with proxy reads and an explicit update hook.
| File | Description |
|---|---|
src/colvars_system.h |
Exposes boundary fields and comments out unsupported-boundary handling. |
src/colvarproxy_system.h |
Adds a boundary-update hook and const-reference accessor. |
src/colvarmodule.h |
Declares atom-group registration and tracking state. |
src/colvarmodule.cpp |
Centralizes group preparation, gradient stages, and GPU force submission. |
src/colvarcomp.h |
Removes component-level preparation and fitting-gradient APIs. |
src/colvarcomp.cpp |
Registers component groups and removes relocated processing. |
src/colvarcomp_distances.cpp |
Reads boundary conditions locally for distance calculations. |
src/colvarcomp_coordnums.cpp |
Reads boundary conditions locally for CPU and GPU coordination calculations. |
src/colvarcomp_angles.cpp |
Reads boundary conditions locally for angle calculations. |
src/colvar.h |
Declares a separate component-gradient debugging method. |
src/colvar.cpp |
Removes relocated stages and separates gradient debugging. |
namd/cudaglobalmaster/colvarproxy_cudaglobalmaster.C |
Implements explicit, synchronized lattice updates for NAMD. |
Review effort: Balanced
Comment on lines
+397
to
+399
| for (size_t i = 0; i < atom_groups.size(); i++) { | ||
| cvmodule->unregister_atom_group_from_cvc(atom_groups[i]); | ||
| } |
Comment on lines
+969
to
+971
| for (auto cvi = variables_active()->begin(); cvi != variables_active()->end(); cvi++) { | ||
| if (!(*cvi)->is_enabled(colvardeps::f_cv_total_force_current_step)) { | ||
| error_code |= (*cvi)->calc_cvc_total_force(0, (*cvi)->num_cvcs()); |
Comment on lines
+1014
to
+1016
| it->first->reset_atoms_data(); | ||
| it->first->read_positions(); | ||
| it->first->calc_required_properties(); |
| } | ||
| error_code |= proxy->update_system_boundaries(); | ||
| decrease_depth(); | ||
| error_code |= calc_colvars(); |
Comment on lines
+1045
to
+1046
| if ((*cvi)->is_enabled(colvardeps::f_cv_total_force_current_step)) { | ||
| error_code |= (*cvi)->calc_cvc_total_force(0, (*cvi)->num_cvcs()); |
Comment on lines
+1355
to
1357
| for (auto ag = registered_atom_groups.begin(); ag != registered_atom_groups.end(); ++ag) { | ||
| error_code |= ag->first->get_gpu_atom_group()->add_force_to_proxy_gpu(ag->first); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR implements the idea discussed in #938 (comment), and also refactors the update of lattice (boundary condition) so that
boundaries_is updated only once in the GPU code path. This PR also lays the foundation of reusing atom groups.Related issue(s)
N/A
Affected engine(s)
All.
Author(s)
@HanatoK
Licensing
By submitting this pull request, I agree that my contribution will be included in Colvars repository and redistributed under the GNU Lesser General Public License (LGPL) version 3.
Note: NAMD, VMD and TinkerHP follow more restrictive licenses, e.g. the UIUC license. However, the terms of the LGPLv3 remain in effect for the copies of the Colvars code distributed alongside those packages.
Artificial Intelligence (AI)
No AI is used so far.
Further information / links
Supersede #953