Add support for the arithmetic vdw rule#78
Add support for the arithmetic vdw rule#78David-Araripe wants to merge 16 commits intofeature/qgpufrom
Conversation
…le (as supported by the fortran code)
There was a problem hiding this comment.
Pull request overview
This PR adds support for the arithmetic van der Waals (vdW) combination rule to Q-GPU, which previously only supported the geometric rule. The arithmetic rule uses arithmetic averaging for atomic radii (R*_ij = R*_i + R*_j) combined with geometric averaging for epsilon parameters (eps_ij = sqrt(eps_i * eps_j)).
Changes:
- Added vdw_rules.h header file defining geometric and arithmetic vdW calculation functions
- Added vdw_rule field to system topology structure and CSV I/O
- Updated all nonbonded force calculation functions to support both rules with runtime selection
- Added parameter preprocessing to convert epsilon values to sqrt(epsilon) for arithmetic rule
- Updated Python topology parser to read and validate vdW rule specifications
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/core/vdw_rules.h | New header defining inline functions for geometric and arithmetic vdW calculations with CUDA support |
| src/core/system.h | Added vdw_rule integer field to topo_t structure |
| src/core/parse.cu | Added vdw_rule reading, validation, and parameter preprocessing for both catypes and q_catypes |
| src/core/solvent.cu | Updated water-water interactions to support both vdW rules with optimized coefficient precomputation |
| src/core/qatoms.cu | Updated Q-atom interactions (QP, QW, QQ) to dispatch to appropriate vdW calculation function |
| src/core/nonbonded.cu | Updated protein-protein interactions to support both vdW rules |
| src/core/cuda/src/CudaNonbondedForce.cu | Updated CUDA kernel to pass vdw_rule parameter and call appropriate calculation function |
| src/Qgpu/topology.py | Enhanced topology parser to detect vdW format, validate consistency, and write vdw_rule to CSV |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/core/vdw_rules.h
Outdated
| *V_a = sqrt_eps_ij * R6 * R6 * r6 * r6; // eps * R^12 * r^-12 | ||
| *V_b = 2.0 * sqrt_eps_ij * R6 * r6; // 2 * eps * R^6 * r^-6 |
There was a problem hiding this comment.
The inline comment states "eps * R^12 * r^-12" but the code uses sqrt_eps_ij which equals sqrt(eps_i * eps_j), not eps_ij. The comment should clarify this is sqrt(eps_i * eps_j) rather than eps_ij. Similarly for line 49, the comment should specify this is 2 * sqrt(eps_i * eps_j), not 2 * eps_ij.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@David-Araripe I've opened a new pull request, #79, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: David-Araripe <79095854+David-Araripe@users.noreply.github.com>
Co-authored-by: David-Araripe <79095854+David-Araripe@users.noreply.github.com>
The cdk2 test is passing just fine. I tested it with:
The thrombin example seems to have a bug or something different in the implementation. See the output for thet test run:
Where we get:
Larger differences are observed for
nonbonded pp vdwand forrestraint Ushell. That's something we should investigate.One suggestion I would have is making the tests slightly more permissive for declaring "Energy not matching". We have this message even with 0.01 difference. I don't know how permissive we'd like to be, though.. If you let me know I can include this change to this PR.