Repository navigation
Conversation
Every line is a row of 16-character, left-aligned cells: the tag, then KEY/unit and value cells, so every cell starts on a column edge and the columns line up across lines and iterations. Values are in scientific notation (9 significant digits for energies, 8 otherwise). The PCC moments form a table whose headers start at the column edges. PCC_USED/Ry is dropped (it repeats E_PCC/Ry), and SCCS_DEFERRED is formatted in sccs_output.cpp with the other lines.
47b8635 to
434ee60
Compare
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
The new solvent-output regression can pass when an expected cube is missing; please make that absence fail the check.
Non-blocking: please record the exact commands, configurations and observed results for the added unit/integration tests and relevant INPUT checks, identifying anything not run.
Static review of the complete feature delta at 434ee60e3e5005782a8f73970722c41fd32555cb; builds, tests and numerical runs were not executed, and CI diagnostics were unavailable.
| if [[ "$out_sol" == "1" && -f "OUT.autotest/sol_cavity.cube" ]]; then | ||
| integral=$(python3 "$CUBE_TOOL" integrate "OUT.autotest/sol_cavity.cube") | ||
| echo "sol_cavity_integralref $integral" >>$props_result_file |
There was a problem hiding this comment.
This condition silently skips sol_cavity_integralref when the cube is missing, while Autotest.sh compares only keys emitted in result.out. Please fail when a case expecting that reference has no cavity cube, and assert that both expected cube files exist even though the dielectric integral is intentionally omitted.
There was a problem hiding this comment.
With out_sol 1, the collector now requires both sol_eps.cube and sol_cavity.cube and exits 1 otherwise, like the out_alllog check in the same file, so catch_properties.sh reports a fatal error. The dielectric integral is still only checked for presence. I checked this on relax_sccs: with sol_eps.cube removed it exits 1, and with the file restored it exits 0.
| para.input.out_sol[0] = std::stoi(item.str_values[0]); | ||
| if (count >= 2) | ||
| { | ||
| para.input.out_sol[1] = std::stoi(item.str_values[1]); |
There was a problem hiding this comment.
Non-blocking: out_sol 1.5 6junk is parsed as 1 6, and tokens after the second are ignored. The vector-domain binding does not validate these original tokens; please require exactly one or two complete integer tokens and cover malformed prefixes and extra arguments in the INPUT tests.
There was a problem hiding this comment.
out_sol takes exactly one or two tokens, and each must be a whole integer: std::stoi with the position argument, and the token must be fully consumed.
| ModuleSccs::SccsResponse response; | ||
| ModuleSccs::solve_response(density, solute_charge, config_.cavity, solver_, restart_potential_, basis, | ||
| cell->tpiba, *coulomb, response); | ||
| output_.transforms = coulomb->fft_counts(); |
There was a problem hiding this comment.
Non-blocking: these counters cover only Coulomb applications inside solve_response; the following eval_functional call and direct cavity/diagnostic transforms are omitted. Please label/document that scope, or instrument the full scope if total SCCS FFT counts are intended.
There was a problem hiding this comment.
The counters are meant to measure the sqrt-CG solve, which is the main cost. Reading them after eval_functional would still miss the cavity-derivative FFTs, so I now document the scope instead: a comment at the read, and the sccs_debug help text now says "the FFT counts of the Coulomb operator in the sqrt-CG solve".
| for (std::size_t i = 0; i + 1 < cells.size(); ++i) | ||
| { | ||
| line += cells[i]; | ||
| line.append(cell_width - cells[i].size(), ' '); |
There was a problem hiding this comment.
Non-blocking: please assign the padding width to a named local before append, and likewise name digits - 1 before setprecision above. Root AGENTS.md rule 14 requires computed arguments to be named so their values can be inspected at the call site.
There was a problem hiding this comment.
The precision and the padding are now named locals. This also fixed an edge case: a value as wide as the 16-character column, such as an energy with a three-digit exponent, used to run into the next cell.
- props_basic.sh: out_sol 1 fails the case when sol_eps.cube or sol_cavity.cube is missing, instead of silently skipping the sol_cavity_integralref check. - out_sol: accept exactly one or two whole integer tokens; "6junk", "8x" and a third token are rejected. INPUT tests cover these. - sccs_debug: the help text and a comment state that the FFT counts are the Coulomb-operator FFTs of the sqrt-CG solve. - sccs_output: name the computed precision and padding; a cell as wide as the column (a three-digit exponent) keeps one blank instead of running into the next cell. A test covers it.
Linked Issue
Fifth part of #8052. It follows #8075 (PCC), #8162 (SCCS Step 1: periodic SCCS), #8194 (SCCS Step 2: SCCS with PCC 0D/2D) and #8219 (SCCS Step 3: derivative lowpass filter and
fullcavity).This PR is stacked on #8219: until #8219 is merged, please review only the last commit. The remaining follow-up PR adds the solvent-aware cavity.
Summary
Steps 1–3 run SCCS only in a single SCF, with the solvent active from the first iteration and with no diagnostics. This PR adds what is needed to use SCCS in real calculations:
sccs_start_drho,sccs_start_nmax). The SCF first runs without the solvent until reaching a desired drho, then SCCS starts, which is also applied in Environ and cp2k.calculation relax). The SCCS potential is rebuilt every ionic step. The start state and the last sqrt-CG solution carry over as a warm start.sccs_debug 1/2). Solver iterations, residuals, cavity volume and surface, Gauss-law and multipole checks are written to the running log (only for debug).out_sol 1). The dielectric function and the cavity are written as cube files, for bothimp_sol 1andimp_sol 2.All four are off by default, and nothing changes for existing inputs: no existing integration reference changes.
What's changed?
1. Delayed start and fixed-cell relax
PotSccsgains a start state. Until it starts, it contributes no potential, energy or force. It starts once DRHO ≤sccs_start_drho, or at iterationsccs_start_nmaxat the latest. Once started, it stays on.pot_registerreads the SCCS state (SccsResume: start flag and warm start) from the old components before deleting them. It passes the state explicitly toget_pot_type, which gains aconst SccsResume¶meter, so no new member variable holds it. Registration keeps its original order: old components are deleted first.SccsFixedSourcescaches the Gaussian ion and core densities and the grid coordinates within one ionic step. They are rebuilt only when the atoms change.2. Diagnostics (
sccs_debug)3.
out_solout_sol 1 [precision]writessol_eps.cubeandsol_cavity.cubethroughModuleIO::write_vdata_palgrid, at the same point asout_pot.imp_sol 1,surchemkeeps the dielectric function of its lastcal_vel, and the cavity is computed from it as 1 − S.Governance Notes
esolver_ks.cpponly). Followingsource/source_esolver/AGENTS.md, it only calls named steps ofmodule_chargeand adds no function of its own:iter_finishcallsupdate_pot_modelbefore mixing andprint_corr_iterafterprint_etot;iter_finishreadsPARAM.globalv.two_fermionce into a local.chg_routine):update_pot_modelandprint_corr_iterselect the first rank of the first pool, the screen for the start notice and the running log forsccs_debug.Potentialgainsmodel_pending,update_model,write_corr_iterandsol_out_fields.get_pot_typegains aSccsResumeparameter, and its only call site is updated.ctrl_output_fpwritesout_sol.GlobalV::ofs_runningfor the running log, inchg_routine.cpp, and thetwo_fermilocal) and removes 3, for a net change of −1. The checker reports these as warnings only.<iosfwd>inpot_sccs.h,potential_new.handsccs_output.h:std::ostreamparameters.sccs_output.handsccs_fixed_src.hinpot_sccs.h: value members ofPotSccs.charge_moments.handsccs_coulomb.hinsccs_output.h: value members ofPccOutputandSccsOutput.<memory>and<vector>insccs_fixed_src.h,<string>and<vector>insol_grid_field.h,<vector>insccs_diag.h,sccs_output.handsurchem.h.esolver_ks.cppis 65, as ondevelop