Repository navigation
Conversation
HanatoK
force-pushed
the
refactor_atom_groups_read_data
branch
from
August 17, 2026 16:41
e19dc7c to
ab69a1d
Compare
HanatoK
marked this pull request as ready for review
August 18, 2026 19:41
Merged
HanatoK
commented
Sep 1, 2026
HanatoK
force-pushed
the
refactor_atom_groups_read_data
branch
from
September 1, 2026 21:43
ab69a1d to
f65ae75
Compare
HanatoK
marked this pull request as draft
September 1, 2026 22:15
Member
Author
|
I will look into the test failures in https://github.com/HanatoK/colvars/actions/runs/33562573020/job/100038292990 tomorrow. |
HanatoK
marked this pull request as ready for review
September 2, 2026 20:02
HanatoK
force-pushed
the
refactor_atom_groups_read_data
branch
from
September 10, 2026 15:01
0e80deb to
17ab353
Compare
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.
HanatoK
force-pushed
the
refactor_atom_groups_read_data
branch
from
November 12, 2026 21:39
17ab353 to
f1fac18
Compare
Member
Author
|
Close by #999 |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved lifetime and evaluation-order defects can cause use-after-free crashes and produce stale forces and gradients.
7 open issues
What changed in this PR
Refactors Colvars’ atom-group preparation into module-level scheduling, laying groundwork for shared atom groups and a single GPU boundary update per evaluation. It builds on the stream-based design in #938 and depends on #940.
Changes:
- Adds a central atom-group registry and moves data reads, fit-gradient calculation, and GPU force submission into the module.
- Replaces per-component boundary caches with proxy-managed boundary state.
- Separates gradient debugging from component-gradient calculation.
| File | Description |
|---|---|
src/colvars_system.h |
Exposes boundary fields and disables the unsupported-boundary check. |
src/colvarproxy_system.h |
Adds an explicit boundary-update hook and const-reference getter. |
src/colvarmodule.h |
Declares the atom-group registry, counters, and synchronization state. |
src/colvarmodule.cpp |
Centralizes group preparation, fit gradients, force handling, and boundary updates. |
src/colvarcomp.h |
Removes component-level preparation APIs and the boundary cache. |
src/colvarcomp.cpp |
Adds registry lifecycle calls and removes component preparation routines. |
src/colvarcomp_distances.cpp |
Reads proxy boundary state directly in distance calculations. |
src/colvarcomp_coordnums.cpp |
Uses proxy boundary state in CPU and GPU coordination calculations. |
src/colvarcomp_angles.cpp |
Reads proxy boundary state in angle and dihedral calculations. |
src/colvar.h |
Declares the separate gradient-debugging method. |
src/colvar.cpp |
Removes relocated preparation stages and separates gradient debugging. |
namd/cudaglobalmaster/colvarproxy_cudaglobalmaster.C |
Implements explicit lattice synchronization and boundary updates. |
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
+1024
to
+1025
| // Calculate atom group fit gradients | ||
| if (use_gpu) { |
| error_code |= (*cvi)->debug_cvc_gradients(); | ||
| } | ||
| 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
+1356
| 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); |
| } else { | ||
| (cvcs[i])->read_data(); | ||
| } | ||
| if (use_gpu && cvcs[i]->is_enabled(f_cvc_support_gpu)) { |
Comment on lines
+978
to
+992
| for (auto it = registered_atom_groups.begin(); it != registered_atom_groups.end(); ++it) { | ||
| auto parent_cvcs = it->first->get_parents(); | ||
| bool use_cpu_buffers = false; | ||
| for (auto c = parent_cvcs.begin(); c != parent_cvcs.end(); ++c) { | ||
| if ((*c)->get_object_type() == colvardeps::object_t::colvarcomp) { | ||
| auto cvc = dynamic_cast<colvar::cvc*>(*c); | ||
| use_cpu_buffers |= cvc->is_enabled(colvardeps::f_cvc_require_cpu_buffers); | ||
| } | ||
| } | ||
| if (use_cpu_buffers) { | ||
| it->first->reset_atoms_data(); | ||
| } | ||
| error_code |= it->first->get_gpu_atom_group()->read_data_gpu(it->first); | ||
| error_code |= it->first->get_gpu_atom_group()->after_read_data_sync(it->first, use_cpu_buffers); | ||
| } |
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.
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.This PR depends on #940. Sadly I still haven't figured out how to make a stacked PR...