-
Notifications
You must be signed in to change notification settings - Fork 1
Add support for the arithmetic vdw rule #78
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
base: feature/qgpu
Are you sure you want to change the base?
Conversation
…le (as supported by the fortran code)
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.
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 |
Copilot
AI
Jan 28, 2026
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 <[email protected]>
Co-authored-by: David-Araripe <[email protected]>
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.