-
Notifications
You must be signed in to change notification settings - Fork 9
Implementation of the charge gradient dqdr for EEQ-BC with unit test and speed up for solve without gradient #48
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
Conversation
If the charge gradient is not requested via lgrad, the set of linear equations is now solved directly.
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 implements analytical charge gradients (dqdr) for the EEQ-BC charge model, adds comprehensive unit tests including numerical gradient validation, and optimizes the charge equation solver to avoid computing matrix inverse when gradients are not requested. The implementation adds EEQ-BC specific variables that are conditionally allocated based on the model type and whether gradients are needed.
Key changes:
- Implements analytical gradient calculation for EEQ-BC partial charges with respect to atomic positions
- Adds
BLAS_SolveSymmetric()to solve linear systems directly without matrix inversion whenlgrad=false, improving performance - Introduces new test molecule (amalgam) and reference gradient data for validation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_multi.h | Adds reference gradient data (dqdr_reference) and reference charges for amalgam test molecule |
| test/test_multi.cpp | Adds test_eeqbc_amalgam() test function and updates existing tests to request gradients |
| test/test_grad.cpp | Splits gradient testing into separate functions for EEQ and EEQ-BC models, adds EEQ-BC numerical gradient test |
| test/molecules.h | Defines new 56-atom amalgam test molecule with diverse element types |
| src/dftd_ncoord.cpp | Fixes coordination number gradient calculation with missing f_directed factors and normalization term |
| src/dftd_eeq.cpp | Implements gradient calculations for EEQ-BC including new helper functions get_xvec_derivs(), get_dcmatdr(), get_dcpair(); optimizes solver for non-gradient case |
| include/dftd_eeq.h | Updates function signatures to support gradient calculations with new parameters |
| include/dftd_cblas.h | Adds BLAS_SolveSymmetric() function using Bunch-Kaufman factorization for symmetric linear systems |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
The pre-commit.ci is failing since it is implemented after this commit (.pre-commit-config.yaml is missing). Not sure why it is running, but we can just ignore the failed check. See #49 |
precommit has to be enabled globally. That's why the CI started in this PR as well. |
|
I think we can merge this, right? |
I implemented the charge gradient dqdr for EEQ-BC, which is tested in the test_grad against the numerical gradient.
The "solve" function, here called "eeq_chrgeq", includes EEQ-BC specific variables that are given to ChargeModel functions even if the EEQ model does not require them. However, these variables are only allocated if they are actually needed.
Additionally, if the charge gradient is not requested via lgrad, the set of linear equations is now solved directly (which avoids calculating the inverse of the Coulomb matrix).