Repository navigation
Conversation
|
@ABACUS-PR-BOT Could you review the PR? |
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Independently reviewed statically at 7cdc76650742af3d40120d8969465e46bce44172. The numerical finding is an analytic finite-grid limitation; molecular impact has not been measured. No builds, runtime/numerical jobs, or CLI/doc-generation checks were run; the required CI connector was unavailable. The trusted governance check exited 0 with warnings; git diff --check exited 2 for two added blank EOF lines.
Non-blocking (verification record): Please add the exact commands, configuration/executable identity and observed results for the SCCS unit/MPI/integration checks and any Environ comparison. The PR reminder is checked but the body has no verification section. Also record documentation-generation verification or why it was not run; those generated files are intentionally untracked.
| { | ||
| const ModuleBase::Vector3<double>& gradient = polarization.gradient[i]; | ||
| const double gradient_square = gradient * gradient; | ||
| response.cavity_potential[i] = -response.depsilon_drho[i] * gradient_square / (8.0 * ModuleBase::PI); |
There was a problem hiding this comment.
[P2] Numerical limitation: this continuum cavity derivative is not generally the derivative of the implemented finite-grid reaction energy. At uniform density inside the switching interval, the latter gives epsilon_prime/(16*pi) * (2*phi*L(phi)-L(phi^2)); on an eight-point periodic grid, a neutral cos(3x) potential and zero-mean cos(2x) density perturbation distinguish the two because phi^2 aliases. Please add a varying-dielectric density finite-difference check with grid refinement and either make the discretization variational or bound/document the approximation; tighter CG tolerances alone do not remove it. This is an analytic counterexample, not a measured error in the water case.
There was a problem hiding this comment.
Confirmed. This is the same continuum derivative QE-Environ uses (−ε'|∇φ|²/8π with FFT gradients). I have tried other methods, including introducing the exact derivative, but the tests showed that some new numerical problems might be involved. And this problem has to be resolved in the low-pass parts (same as QE).
| response.dsolute_drho.resize(size); | ||
| response.epsilon.resize(size); | ||
| response.depsilon_drho.resize(size); | ||
| response.grad_log_epsilon.resize(size); |
There was a problem hiding this comment.
Non-blocking: grad_log_epsilon is allocated and filled but has no reader in this revision; the coefficient uses local_gradient directly. Deferring this field would save three doubles per local FFT point and its write pass on each SCCS update. This cleanup can be a follow-up.
There was a problem hiding this comment.
The field is consumed by the SCCS diagnostics in the stacked follow-up PR (SCF integration, start SCCS at a desired drho), so I will keep it here to avoid churning the field across the stack.
|
The SCCS in the current PR is not fully ready, as it requires PCC for charged systems and a low-pass filter for cavity forces in the following PRs. |
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.source/changes.Linked Issue
Second part of #8052, split according to the review feedback.
Follow-up PRs, each stacked on the previous one: PCC coupling → lowpass and full cavity → SCF integration → solvent-aware cavity.
What's changed?
imp_sol 2selects the self-consistent continuum solvation model (SCCS; Andreussi, Dabo and Marzari, J. Chem. Phys. 136, 064102 (2012)) for neutral, periodic cells, withcalculation scf, energies and forces, PW and LCAO,nspin1 and 2. The original ABACUS solvent model staysimp_sol 1, andtrue/falseare still read as 1/0.sccs_rho_minandsccs_rho_max.sccs_presetprovides Environ'svacuum,water-neutral,water-cationandwater-anionparameters;customusessccs_epsilon,sccs_rho_min/max,sccs_gammaandsccs_pressure.New code:
source/source_hamilt/module_sccs/(cavity, presets, periodic Coulomb operator, Gaussian ions and forces, sqrt-CG response, energy functional), thePotSccspotential component insource_estate/module_pot/, andread_inp_sccs.{h,cpp}for the INPUT keys.Deliberate differences from Environ:
-div(grad s / |grad s|_eta), instead of Environ's unregularized curvature formula. With a density saddle on a grid point (He2), QE-Environ's surface potential reaches 100-3000 times its median and 4 of 6 runs do not converge in 200 steps; ABACUS converges in 5. So this approach seems better.sccs_tol_rmsandsccs_tol_max. Environ stops on the unnormalizedsum(r^2), so its accuracy depends on the grid size.Governance Notes
imp_solbecomes an Integer (0/1/2);true/falseremain valid.sccs_preset,sccs_epsilon,sccs_rho_min,sccs_rho_max,sccs_gamma,sccs_pressure,sccs_maxiter,sccs_tol_rms,sccs_tol_max,sccs_surface_eta. They are listed right after the original solvent keys.PotBasegains three virtual hooks with empty defaults:get_solvation_energy,add_solvation_forceandsolvent_electrostatic_potential.Potentialforwards them to its components. No existing component overrides them, so other calculations are unchanged.Potential::pot_register/get_pot_typetakeconst Input_para&explicitly; the old one-argumentpot_registerforwardsPARAM.inp.get_pot_typenow readsgga_gradanddip_cor_flagfrom that argument instead ofPARAM.inp.ElecState::cal_energiestakesimp_solasint;imp_sol 2reads the energies from the potential.force_pw.cpp) and LCAO (force_stress_lcao.cpp) add the solvation force throughPotential::add_solvation_force;imp_sol 1keeps its own path.