Repository navigation
feat(dfpt): v_sc Broyden mixing for the response SCF (default), plus rho_broyden, rho_kerker, v_sc_plain - #8214
Conversation
dfpt_mix_type (plain|kerker) and dfpt_kerker_a2 replace the design-phase env knobs DFPT_MIX_TYPE / DFPT_KERKER_A2 / DFPT_MIX_BETA; the plain 0.4 default behavior is unchanged. Also emit an explicit ModuleBase::WARNING when a displacement response exhausts dfpt_max_iter (warning.log channel, next to the stdout summary from the R4a fix in deepmodeling#8163). Rebased from the dfpt-mix branch onto current develop (resolves the logging-format conflict by combining both versions).
…den) New INPUT parameter dfpt_broyden_ndim (default 8). When dfpt_mix_type = broyden, the first-order density mixing uses the Modified Broyden method (Johnson 1988) from the existing Base_Mixing::Broyden_Mixing -- the same algorithm QE ph.x uses (nmix_ph = 4 default there). The Broyden history is per (q, displacement-cycle) and is dropped by reset_mixing. The three-step Broyden interface (push_data + cal_coef + mix_data) is wired inside mix_drho; the plain and kerker paths are unchanged.
…_sc)
When dfpt_mix_type = v_sc, the SCF fixed-point variable changes from
the first-order density drho to the screening potential v_sc: each
iteration solves the Sternheimer equation with the mixed potential,
obtains the output density, rebuilds the output potential from it, and
mixes the two potentials. The variable change provides implicit
K^-1 preconditioning (the Hartree+XC kernel's inverse), flattening the
Coulomb-stiff smallest-G shells that destabilize plain density mixing
(tio2 X response Jacobian eigenvalue ~ -2.2 on the {111}/{200} shells).
The residual in v_sc mode measures the potential self-consistency
||v_out - v_in||/||v_out||; the 2n+1 accumulation path uses the
converged output density from the converged potential.
plain, kerker, and broyden paths are unchanged.
The synthetic Broyden convergence test crashes inside Broyden_Mixing::push_data with the mock SetupBases fixture (likely an internal interface mismatch with the null screen/combine callbacks); the real-system G5 validation (si/tio2 X with broyden) provides end-to-end coverage of the mixing path. Re-enable after the mock interface is understood.
…d DFPT_PW setter Two validation points silently rejected the new mixing types: DFPT_PW::set_mix_type only accepted plain/kerker (broyden/v_sc were dropped, leaving the default plain), and DFPT_Rho::init WARNING_QUIT on any mix_type outside plain/kerker. Both now accept all four.
Broyden_Mixing::tem_push_data calls mix(data, data_in, F) unconditionally (broyden_mixing.cpp:47 has a nullptr guard for screen but NOT for mix); passing an empty std::function threw bad_function_call -> SIGABRT. Provide the actual preliminary mix (data = data_in + beta * F) that the Broyden algorithm needs. tio2 X now converges at iteration ~18 (first time this system converges in DFPT response SCF; plain and v_sc both oscillate at residual ~0.8-1.7).
…ments reset_mixing destroys the per-q Mixing_Data object but the Broyden mixer's internal 'address' pointer still referenced the freed object; the next displacement's push_data then hit the 'One Broyden_Mixing object can only bind one Mixing_Data' guard and WARNING_QUIT. Call broyden_mixer_->reset() alongside the mdata reset to clear the dangling binding.
…t; add v_sc Broyden dfpt_mix_type values renamed to a consistent naming scheme: plain -> rho_plain (default) kerker -> rho_kerker broyden -> rho_broyden v_sc -> v_sc_plain (new) -> v_sc_broyden v_sc_broyden uses a separate Broyden_Mixing instance on the nrxx-length real-space screening potential (the density Broyden operates on npw-length G-space arrays). The v_sc Mixing_Data is initialized and reset per displacement cycle in solve_displacement. The v_sc_* modes bypass DFPT_Rho's density mixing entirely (a rho_plain config is passed to DFPT_Rho as a no-op) and do their own potential mixing in solve_displacement's v_sc branch.
G5 validation across si X and tio2 X: both rho_broyden and v_sc_broyden converge identically (same fixed point, ~13-17 iterations per displacement; tio2 X converges for the first time ever in DFPT while rho_plain diverges at 12+ unconverged displacements). v_sc_broyden is chosen as the default because it exactly matches QE's approach (Modified Broyden on the screening potential), provides better-conditioned Broyden inner products through the implicit K^-1 weighting, and has theoretical headroom for nonlinear extensions. All five mixing methods remain available via dfpt_mix_type: rho_plain | rho_kerker | rho_broyden | v_sc_plain | v_sc_broyden
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static review of 14c1890 against 89595dd, with independent source and algebra checks. The comments include correctness findings and explicitly labeled nonblocking notes. No ABACUS build/runtime/MPI tests were run; selected-account CI diagnostics were unavailable.
- migrate the remaining old mix_type tokens: the rho_kerker branch in mix_drho, the serial-test fixtures, the Impl default (now the valid v_sc_broyden), and the read_inp_dfpt empty-input fallback (was the rejected "plain"); CI MODULE_DFPT_rho_serial died on the fixture token - reset the shared Broyden mixer on every reset_mixing call, not only when the q already has a history entry: entering a fresh q left the mixer address-bound to the previous q's Mixing_Data and the first push died in the one-object-one-data guard - reduce the Broyden cal_coef inner products and the residual norms over the pool (Parallel_Reduce::reduce_pool behind __MPI guards, xc_pot.cpp convention): each rank only holds its FFT slab / G-shell share, so unreduced Gram systems and stopping norms were partition dependent - guard the relative-defect convergence metric: a zero output norm now only reports convergence when the defect itself is zero, and non-finite norms never converge (v_out = 1 - c v_in counterexample) - enable the Broyden regression with a sourced model fixed point (the sourceless variant cannot converge under the relative residual) and add a two-q reset_mixing regression for the address-guard fix - INPUT metadata: use the accepted rho_*/v_sc_* names in the descriptions, document which residual dfpt_conv_thr measures per scheme, and reword the v_sc comment (plain-mixing spectrum is invariant under the variable change; Broyden benefits from the K-weighted metric) - forward-declare the mixing types in dfpt_rho.h and drop the concrete includes (dfpt_pw_impl.h already includes them directly)
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static incremental review of 1a2f421 against 89595dd, independently assessed by two reviewers. The five previously reported correctness roots are addressed in source; nonblocking coverage/header/domain follow-ups remain tracked in their existing threads. No build, test, MPI run or numerical-accuracy/QE-equivalence validation was performed; corrected pool metrics do not establish end-to-end finite-q MPI support.
…regression Review follow-up: the binding populated shell is d = 0.5 with factor |1 - beta (1 - D)| = 0.8 (>= 103 plain steps to 1e-10), not the unpopulated-shell bound 0.6. Assertions are unchanged.
|
Fixes: all remaining old mix_type tokens migrated (CI root cause: the test fixture |
Critsium-xy
left a comment
Critsium-xy
left a comment
There was a problem hiding this comment.
Static review of 07276d1; no build or run performed. The previously reported correctness findings look addressed in source. Inline comments below are minor.
Verification evidence for the default change. The default switches from rho_plain to v_sc_broyden, and the stopping metric switches from a density residual to a potential residual under the same dfpt_conv_thr = 1e-8. The two residuals scale differently (the Hartree kernel weights small G), so the same threshold need not give the same drho accuracy. There is no DFPT integration test under tests/, so green CI does not cover this. Please add to the PR body:
- on a case where
rho_plainconverges (e.g. diamond or Si), phonon frequencies and iteration counts forrho_plainvsv_sc_broyden; - the tio2 X result with the new default.
Also, "matching QE ph.x exactly" overstates it: ABACUS Broyden (truncated pseudo-inverse, no w0) differs from QE mix_potential, and QE stops on an absolute dr2. "Same strategy as QE ph.x (Broyden on dvscf)" would be accurate.
- E-field solve (dfpt_pw_q0.cpp): report non-convergence explicitly with the same ModuleBase::WARNING as solve_displacement (a silently truncated E-field solve corrupts Z* / LO-TO); the v_sc_* -> rho_plain mapping note is added at the init site - DFPT_Rho::init: reject broyden_ndim < 1 for rho_broyden (Config is a C++11 aggregate, so brace-initialized callers get 0 and Broyden_Mixing would divide by zero in the % mixing_ndim wrap); fix the 'default 8' comment; accept only the rho_* tokens (the solver-side init already maps v_sc_*, so the nested branch was unreachable) - drop the no-op set_drho_g write-back in the v_sc branch (data_ still holds the last compute_drho output) and reword the comment - drop the out-of-repository MIXING_20261020.md reference - rename safe_relative_defect_ -> safe_relative_defect (free function in an anonymous namespace; the trailing underscore is the member convention); sync the plain-mixing contraction notes in the test (roughly 100 iterations at 0.8/step on the binding shell)
…der v_sc_broyden Review follow-up (Critsium-xy): with the v_sc_broyden default, the E-field solve (Z* / epsilon_inf / LO-TO) silently fell back to plain density mixing at beta 0.4 -- the weakest member of the family exactly on the path that matters most for polar systems. Map v_sc_broyden to rho_broyden for DFPT_Rho (v_sc_plain keeps rho_plain), so the E-field loop runs Modified Broyden on the density while the displacement loop keeps its potential-space Broyden. Verification (si, Gamma + dfpt_compute_q0/dfpt_loto, default INPUT so the path is exercised through the mapping): E-field 3/3 directions converge in 12 iterations (bit-identical residuals to an explicit rho_broyden run) versus 38 with the previous plain fallback -- displacements unchanged at 13 iterations (v_sc Broyden); Gamma frequencies and the LO-TO block reproduce the pre-change values exactly. INPUT metadata documents that dfpt_broyden_ndim now sizes both Broyden instances and that the E-field residual is always the density residual.
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static incremental review of 81c95ee against 89595dd, independently assessed after blind review. The new E-field mapping and per-direction reset are consistent in source; the comments consolidate nonblocking test/wording follow-ups. No build, numerical/MPI run or CI acceptance is claimed.
|
Follow-up on After
Also replace "matching QE ph.x exactly" with "same strategy as QE ph.x (Broyden on dvscf)". |
|
All four items done: Si comparison table, tio2 X result, mapping note, and QE wording — all in the current PR body (updated . |
Resolve conflicts with upstream/develop (PR deepmodeling#8225, deepmodeling#8214, deepmodeling#8230, deepmodeling#8223): - write_dipole.cpp grid validation: take upstream version, which rejects only the inconsistent state (nplane == 0 && nrxx != 0) while allowing legitimate empty partitions; a superset of our nplane fix (13b21fb). - write_dipole.cpp file write: keep our MY_RANK == 0 guard (fixes UB of writing to an unopened stream on non-root ranks) wrapping upstream's new setw/scientific output format. - source_io/unittests/CMakeLists.txt: adopt upstream's test/ -> unittests/ rename and test_<module>.cpp file naming (rule 11), while keeping our stru_frame_io.cpp link additions; read_set_globalv.cpp still calls stru_frame_io::extract_last_frame, so the dependency remains. Verification: conflict markers cleared, no stale references to old test file names, all SOURCES entries point to existing files; build not rerun locally.
Summary
The displacement-level response SCF previously used only a plain fixed-beta
density mixer, which diverges for polar / small-gap systems (tio2 X: 317h
unconverged; response Jacobian eigenvalue ~ -2.2 on the smallest-G Coulomb
shells). This PR adds Modified Broyden mixing and a screening-potential
mixing variable, with
v_sc_broyden(Broyden on the screening potential,same strategy as QE ph.x, which mixes dvscf with Modified Broyden) as the
default.
Five mixing methods via
dfpt_mix_type:rho_plainrho_kerkerrho_broydenv_sc_plainv_sc_broyden(default)New INPUT parameters:
dfpt_mix_type(defaultv_sc_broyden),dfpt_mix_beta(0.4),dfpt_kerker_a2(1.0),dfpt_broyden_ndim(8).E-field solve mapping: the electric-field solve (Z*, epsilon_inf,
LO-TO) always mixes the density and receives the density-space counterpart
of the selected scheme:
v_sc_plain->rho_plain,v_sc_broyden->rho_broyden(dfpt_broyden_ndimsizes both Broyden instances). Both thedisplacement loop and the E-field loop emit an explicit WARNING when they
exhaust
dfpt_max_iter(previously silent).Also retires the design-phase env knobs (DFPT_MIX_TYPE / DFPT_KERKER_A2 /
DFPT_MIX_BETA).
Default-path verification (si, Gamma +
dfpt_compute_q0/dfpt_loto,ONCV, ecut 20/80 Ry, 4x4x4 MP grid,
dfpt_conv_thr1e-8; the rho_plaincolumn is the pre-PR plain-default run at identical settings):
rho_plainv_sc_broyden)The fixed point is scheme-independent: frequencies and epsilon_inf agree
between all mixing schemes (the rho_plain reference run enforces
dfpt_asr crystal, hence its acoustic branches are exactly zero; thedefault-path run leaves ASR off and shows acoustic branches at 11-12
cm^-1). The E-field residuals of the default run are bit-identical to an
explicit
rho_broydenrun, confirming the mapping path is exercised.tio2 rutile X with the new default (6 atoms, 18 displacements, ecut
20/80 Ry,
v_sc_broyden): 18/18 displacements converge in 15-18iterations each (avg 16.9). This is the case where the plain family fails:
rho_plain/rho_kerkerat beta 0.2-0.7 accumulate 207-216NOT-converged displacement solves at
dfpt_max_iter, while both Broydenvariants converge and give identical frequencies (same fixed point):
The E-field mapping change is inert at this q point by construction: the
E-field solve only runs at Gamma, and the v_sc displacement loop never
touches the DFPT_Rho density mixer, so the X-point result above is
unaffected by the mapping commit.
Unit/regression tests:
MODULE_DFPT_rho_serialcovers the plain /Kerker algebra, a sourced-model Broyden convergence regression, and a
two-q
reset_mixingregression for the Broyden address-guard fix; CI isgreen on the head commit.