Refactor density matrix module. - #8000
Conversation
…ol flow Mechanical cleanup as the first step of the module_charge governance refactor: convert leading tabs to 4-space indentation (1011 occurrences across 11 files) and add braces around all single-statement if/for/while bodies (11 sites). No functional change.
Introduce a MixingConfig POD that bundles the INPUT mixing parameters with the runtime globals (nspin, scf_thr_type, double_grid), and change set_mixing from a 12-argument interface to set_mixing(const MixingConfig&, double&, double&). Charge_Mixing now stores the config and reads nspin / scf_thr_type / double_grid from it instead of PARAM.inp / PARAM.globalv, removing the direct PARAM reads in set_mixing and init_mixing. The single production call site (esolver_ks.cpp) fills the config, and the unit test drives set_mixing via a make_cfg() helper. The '#define private public' access hack is kept for now with a TODO: the test still must write Parameter::input/sys, Charge::_space_* and XC_Functional privates, which need the Step 4/5 global-state parameterization before it can be removed. Verified: make -j30 MODULE_ESTATE_charge_mixing (build_max_para_test) passes with no errors.
…th std::vector Extract the repeated two-beta mixing functor in mix_rho_recip/mix_rho_real into a make_twobeta_mix<T> template helper (6 lambda copies removed), and convert all local raw new[]/delete[] buffers in charge_mixing_rho.cpp to zero-initialized std::vector, dropping the paired ZEROS calls.
Extend MixingConfig with gamma_only_pw/domag/domag_z so mix_resid.cpp
(get_drho, get_dkin, inner_product_recip_{rho,simple,hartree,real}) no
longer reads PARAM/GlobalV; all branches now consume this->cfg_.
inner_product_recip_rho's raw pointer-array views are switched to
std::vector. Production fills the three new fields in esolver_ks, and
the test fixture gains a sync_cfg() helper to push PARAM mutations into
cfg_ for the inner-product branch tests.
Replace the six private raw _space_rho/_space_rho_save/_space_rhog/ _space_rhog_save/_space_kin_r/_space_kin_r_save buffers with std::vector, so Charge's underlying contiguous storage self-manages and the matching delete[] calls in destroy() (which relied on reading possibly-uninitialized pointers) go away. The public rho/rhog/rho_save/ rhog_save/kin_r/kin_r_save views keep their double**/complex** shape and still alias the vector memory via .data(), so all external consumers are unaffected. Tests that drove _space_* directly are adapted to resize()/.data() and drop their manual delete[] of the buffers.
chgmixing_ks already takes a const Input_para& inp but still read PARAM.inp.mixing_restart / PARAM.inp.scf_nmax from the global. Use the inp argument instead so the function no longer reads INPUT state through the global for these two fields. PARAM.globalv.ks_run is a runtime per-process flag (set from band-parallel topology), not an input, so it is intentionally left as-is rather than threading it through the interface.
init_rho had a cyclomatic complexity of 36 from five sequential stages (file read, atomic fallback, Thomas-Fermi tau, restart load, wfc read) interleaved through shared read_error/read_kin_error flags. Extract the four branches into private methods -- read_rho_from_file, init_rho_atomic_and_tau, load_rho_from_restart, init_rho_from_wfc -- and leave init_rho as a thin sequence of stage calls. Logic is unchanged; the error flags are threaded through as parameters. The deepest stage (read_rho_from_file) now sits at complexity 19, down from 36 for the monolith. The remaining global reads inside the stages are untouched and deferred to a later parameterization step.
…tions sum_rho, cal_rho2ne and non_linear_core_correction each used Charge members only to reach a handful of scalars (nrxx/nxyz/omega) or the reciprocal-shell table (gg_uniq/ngg); the rest of each body is pure numerics. Move the three bodies into a new charge_math namespace as free functions with those values passed explicitly, and leave the Charge members as thin forwarding wrappers so no caller outside the module changes. The kernels are now unit-testable in isolation and no longer coupled to Charge state. One behavior note: the pre-quit debug line that printed sum_rho to ofs_warning is dropped so the free function stays free of global-stream dependencies. charge_math.cpp is wired into the estate library and the charge_test target.
The CMake build already picks up charge_math.cpp; mirror that in Makefile.Objects so the legacy Makefile flow links the new charge_math kernels too. The module_charge directory is already on VPATH, so adding charge_math.o to the object list is sufficient.
…ction Remove Charge::atomic_rho entirely and replace all call sites with module_charge::atomic_rho(..., rhopw), eliminating the need for a thin wrapper on the Charge class. This decouples atomic density initialization from Charge's state and improves charge.cpp quality score from 2 to 44.
scf_out_chg_tau aborted in Parallel_Grid::reduce on assert(rhoin != nullptr) because the kin_r_save[is] handed to write_vdata_palgrid was not a valid buffer. After the _space_* storage became std::vector (ecf5084), a copied/moved Charge leaves its rho/kin_r views dangling into another object's vector buffer, and a kin_r_save never allocated (ked_flag set after allocate) stays nullptr; both surface as a null rhoin deep inside MPI gather instead of at the source. Delete Charge's copy constructor/assignment so any value copy of the vector-aliasing views fails at compile time, and check kin_r_save in ctrl_output_fp before writing tau.cube so a missing allocation reports a clear message instead of tripping the MPI assert. Verification: not run locally (per user request, user compiles).
scf_out_chg_tau (LCAO, SCAN, out_chg=1, 4 MPI ranks) aborted in Parallel_Grid::reduce on assert(rhoin != nullptr). Bisecting between 83eb5d0 (good) and ecf5084 (bad) isolated the regression to ecf5084, which moved Charge's _space_* storage from raw new[] to std::vector. Root cause: with 4 ranks the FFT grid is slab-decomposed so that the last rank owns zero real-space points (nrxx == 0, confirmed via a temporary diagnostic printing fn/is/rank/nrxx at the reduce call site). Before ecf5084, _space_rho = new double[nspin * 0] == new double[0] returned a unique non-null pointer, so rho_save[is] was non-null and the assert passed. After the change, an empty vector's .data() returns nullptr, so the rank with nrxx == 0 handed a null rhoin to reduce and tripped the assert (Debug) or fed MPI_Gatherv a null buffer (Release). A rank with nrxx == 0 is legitimate: MPI_Gatherv is invoked with sendcount 0 and ignores the send buffer. Relax the assert to only flag a null buffer when nrxx != 0, and revert the now-unneeded kin_r_save guard in ctrl_output_fp (it would have falsely aborted on the nrxx == 0 rank). Verification: Release build (build_max_para_test), ran cd tests/03_NAO_multik/scf_out_chg_tau && OMP_NUM_THREADS=1 mpirun -np 4 ../../../build_max_para_test/abacus_max_para Result: exit 0, chg.cube and tau.cube written; numerical comparison against chg.cube.ref/tau.cube.ref gives maxdiff 0 (chg) and 1e-14 (tau).
…ction Move set_rho_core to charge_math::set_rho_core with rho_core, rhog_core and rhopw passed explicitly instead of reading Charge state, and call charge_math::non_linear_core_correction directly. Remove the now-unused Charge::non_linear_core_correction wrapper, use std::vector for the rhocg/vg scratch buffers, update the init_scf call site, and drop the obsolete member stubs in the elecstate unit tests.
Replace the raw new[]/delete[] displacement arrays (dis_old1, dis_old2, dis_now) with std::vector and remove the hand-written destructor. This fixes a read of uninitialized pot_order when an object is destroyed before Init_CE, a memory leak when Init_CE is called repeatedly, and a double-free risk from the implicitly generated shallow copy. The copy constructor and copy assignment are deleted so the molecular-dynamics trajectory history cannot be silently forked. The unit test now checks vector sizes instead of non-null pointers.
- Rename module_charge/charge_math.{h,cpp} to chg_tools.{h,cpp} via git mv
- Change namespace charge_math to module_charge to match charge_atomic
and chgmixing in the same directory
- Update include guard CHG_TOOLS_H and TITLE/timer labels accordingly
- Update call sites in init_scf.cpp, charge.cpp, charge_init.cpp
- Update build references in Makefile.Objects and both CMakeLists.txt
Convert the stateless class Symmetry_rho into namespace module_charge
free functions and rename files for consistency:
symm_rho.{h,cpp} -> chg_symm.{h,cpp}
symm_rho_detail.h -> chg_symm_detail.h
symm_rhog.cpp -> chg_symm_detail.cpp
- 5 public functions become module_charge::symmetrize_rho / cal_rhog_symm
(2 overloads) / cal_rhog_symm_soc (2 overloads)
- 2 cross-TU helpers (psymmg/psymmg_soc) moved to module_charge::detail
via chg_symm_detail.h
- 3 internal MPI helpers moved to anonymous namespace
- Delete dead code psymm (real-space symmetrization, never called)
- Remove empty ctor/dtor and parallel_grid.h include
- Rename begin/begin_soc to cal_rhog_symm/cal_rhog_symm_soc for clarity
- Update timer/TITLE labels from "Symmetry_rho" to "module_charge"
- Migrate all 14 call sites and 1 test stub
- Remove obsolete Makefile special rule (no more name collision)
…uct_recip_simple Move MixingConfig from charge_mixing.h into its own mixing_config.h so stateless residual kernels can include the config without dragging in Charge_Mixing. Remove inner_product_recip_simple, which had no production call sites, together with its unit test.
Relocate gint_prec_ctrl.{h,cpp} and its test into module_gint, update the
include in esolver_ks_lcao.h and rewire the CMake/Makefile object lists.
…ions Rename mix_resid.cpp to chg_drho.cpp and turn inner_product_real and inner_product_recip_hartree into module_charge free functions declared in chg_drho.h; inner_product_recip_rho, which is only shared with the unit test, moves to module_charge::detail in chg_drho_detail.h. Charge_Mixing loses the three private inner-product members and mix_rho_recip/mix_rho_real bind the free functions through lambdas. get_drho/get_dkin stay as members for this step.
Move the get_drho/get_dkin implementations into file-local cal_drho/ cal_dkin free functions with all inputs explicit; the public Charge_Mixing methods become thin forwarding wrappers so esolver call sites stay unchanged.
…nctions
Move Charge_Mixing::Kerker_screen_recip/real to module_charge namespace
as free functions in chg_precond.{h,cpp}, renaming mix_precond.cpp via
git mv. Config/grid/geometry are passed explicitly via MixingConfig,
PW_Basis*, and tpiba, eliminating the function's direct read of
PARAM.inp.nspin. Replace 8 std::bind call sites in charge_mixing_rho.cpp
with lambdas, update 2 commented-out bind sites in charge_mixing_dmr.cpp,
and rewrite 12 test call sites in charge_mixing_test.cpp to construct an
independent MixingConfig instead of poking at Charge_Mixing privates.
Drop the now-unused member function declarations from charge_mixing.h.
…rename Update the non-CMake object list to track the renamed translation unit so make-based builds do not reference the deleted mix_precond.o.
Expose cal_drho/cal_dkin as module_charge free functions in chg_drho.h and let ESolver_KS call them directly with explicit arguments; add Charge_Mixing::get_mixing_config() as a const observer for the config.
Align with the chg_<feature> naming pattern used in the same directory
(chg_drho, chg_precond, chg_symm, chg_tools). Update include guard to
CHG_ROUTINE_H, the self-include in chg_routine.cpp, the entry in
source_estate/CMakeLists.txt and source/Makefile.Objects, and the three
#include sites in esolver_ks{,_pw,_lcao}.cpp. Function names
(chgmixing_ks{,_pw,_lcao}) and TITLE/timer tags are intentionally left
unchanged to keep the diff minimal.
Rename the MixingConfig header to align with the chg_* naming convention in module_charge. Update the include guard and the four in-tree includers; no CMake change is needed since the header is not listed explicitly.
…tions Rename charge_mpi.cpp to chg_parallel.cpp and add chg_parallel.h, moving the three stateless Charge member functions (reduce_diff_pools, rho_mpi, kin_r_mpi) to module_charge namespace free functions that take the Charge object explicitly. Remove their declarations from charge.h and update all call sites in elecstate_pw, stress_mgga, read_wf2rho_pw and sto_iter. Rename the unit test to test_chg_parallel.cpp and update the test target name accordingly. GlobalV/PARAM reads and the direct MPI_Allreduce in reduce_diff_pools are preserved as pre-existing technical debt (migration-neutral).
- Rename module_charge/charge_atomic.{h,cpp} to chg_atomic.{h,cpp}
- Update include guard to CHG_ATOMIC_H
- Update includes in charge_init.cpp and charge_extra.cpp
- Update source paths in CMakeLists.txt, test CMakeLists.txt
- Fix stale object names in Makefile.Objects: replace
symm_rho_charge.o/symm_rhog.o with chg_symm.o/chg_symm_detail.o
…e functions
Introduce module_charge::split_dgrid / merge_dgrid in chg_uspp.{h,cpp} as
RAII, parameter-explicit replacements for Charge_Mixing::divide_data /
combine_data / clean_data, which paired raw new[] with manual delete[]
across ~160 lines of mixing code.
- chg_uspp.{h,cpp}: stateless free functions in module_charge namespace;
outputs are caller-pre-sized std::vector, no new/delete; parameter
validation via WARNING_QUIT; TITLE/timer tags preserved
- charge_mixing_rho.cpp: rho and tau double-grid paths switched to the new
functions; raw pointer aliases kept for !double_grid so the existing
mixing call sites (nspin==1/2/4) are untouched
- CMakeLists.txt (source + test): wire chg_uspp.cpp
The legacy divide_data/combine_data/clean_data members are not yet removed;
that follows in a later step after the test is updated.
In cal_dmr (gamma-only) move the _dmr_ready assignment before timer::end so end is the last statement of the function. In switch_dmr move timer::start to the function entry and timer::end to the function exit, replacing the early-return on spin_mult != 2 with a guard so the timer pair always brackets the full function body, per the ABACUS timer placement rule.
Remove the nested DensityMatrix_Tools namespace and promote all free functions and DmrBlock directly into module_dm, simplifying the naming and eliminating the redundant namespace layer. Affected files: density_matrix.h, dm_tools.cpp, dmr_k.cpp, dmr_td.cpp, dmr_full.cpp, dm_from_psi.h, dm_holder.h, and two unit test files.
The accumulate_dmr template is defined in dmr_k.cpp with only an extern declaration in density_matrix.h. cal_dmr in the same TU implicitly instantiates the combinations it uses, but cal_dmr_td in dmr_td.cpp needs <complex<double>, double, double> and <complex<double>, complex<double>, complex<double>>, which were never emitted. Add explicit instantiations at the end of dmr_k.cpp, matching the existing pattern in dm_tools.cpp.
The module_dm refactor (PARAM elimination) replaced PARAM.globalv.nlocal with pv.nrow in edm_tddft and edm_tddft_lapack. The two values differ under MPI: pv.nrow is the per-process local row count of the 2D block-cyclic distribution (e.g. 5 of 10 orbitals on a 2x2 process grid), while the Scalapack getrf/getri/gemm/geadd calls and the dense gather/LAPACK path need the GLOBAL matrix dimension (desc[2]). The truncated operations silently computed the EDM on only a pv.nrow x pv.nrow global submatrix, losing the remaining orbital contributions. Since the EDM only feeds the overlap force/stress assembly, energies and charges stayed correct while total force and stress deviated (tests/05_rtTDDFT/01_NO_KP_ocp_TDDFT: totalforce 22.34 vs ref 40.75). The error appears from the second MD step on, because the EDM path is gated by istep >= 1. Fix by using Parallel_2D::get_global_row_size(), which returns desc[2] under MPI and falls back to nrow for serial layouts. The same fix applies to the GPU/lapack path (edm_tddft_lapack); that path is compile-verified only, no CUDA runtime available in the CI environment. Verification: - build_basic_para: cmake && make -j 30 abacus_basic_para, 0 errors - OMP_NUM_THREADS=1 ../integrate/Autotest.sh in tests/05_rtTDDFT with the fresh binary: 01_NO_KP_ocp_TDDFT 4/4 OK (etot, etotperatom, totalforce, totalstress all restored)
# Conflicts: # source/source_lcao/module_dftu/test/CMakeLists.txt # source/source_lcao/module_dftu/unittests/test_dftu_nao_op.cpp # source/source_pw/module_pwdft/test/structure_factor_test.cpp
The upstream DFT+U step-11 refactor added module_dftu/unittests/CMakeLists.txt referencing source_estate/module_dm/density_matrix_io.cpp, which was split into dmr_init.cpp / dm_setter.cpp / dm_getter.cpp by ee636ce. Update the source list so CMake can configure the MODULE_DFTU_op test again.
The test drives DensityMatrix::init_dmr/set_dmk/cal_dmr, whose template instantiations live in dmr_gamma.cpp (<double,double>) and dmr_k.cpp (<complex,double> / <complex,complex>). The upstream unittests file only listed density_matrix.cpp, so the linker could not resolve cal_dmr. Add the remaining module_dm sources to match the working module_dftu/test list.
The getter was dead code: no C++ call sites existed. The private member _kvec_d is still used directly by module_dm friend functions in dmr_k.cpp and dmr_full.cpp.
All 23 call sites are refactored to obtain Parallel_Orbitals explicitly: - cal_ldos_lcao: add const Parallel_Orbitals& pv parameter - edm.cpp / force_stress_lcao.cpp: use existing pv reference - esolver_ks_lcao_tddft.cpp: use this->pv - hsolver_lcao.cpp: use this->ParaV member - init_dm: add const Parallel_Orbitals& pv parameter - rpa_lri::cal_postSCF_exx: add const Parallel_Orbitals& parav parameter - exx_lri_interface: add pv parameter to exx_eachiterinit, exx_iter_finish, and exx_after_converge Build verified by user.
Critsium-xy
left a comment
There was a problem hiding this comment.
The 08_EXX CI failure and three further call sites share one root cause: the new DensityMatrix constructor derives quantities that the old one took from the caller.
source/source_estate/module_dm/density_matrix.cpp:38-59:
// old: DensityMatrix(pv, nspin_dm, kvec_d, nk)
// new: DensityMatrix(pv, kvec_d, nspin)
spin_mult = (nspin == 2 ? 2 : 1);
_nk = kvec_d.size() / spin_mult;spin_mult is not a function of nspin. openshell is set by nupdown != 0 or lr_unrestricted (esolver_lr_lcao_tddft.cpp:186,205), both orthogonal to nspin. Two cases already in the suite have the same nspin and require different spin_mult:
| case | nspin |
required spin_mult |
|---|---|---|
tests/08_EXX/51_GO_LR |
2 | 1 (singlet/triplet) |
tests/08_EXX/54_GO_ULR_HF |
2 | 2 (updown) |
The header comment removed by this PR said as much: "not always equal to K_Vectors::get_nks()/nspin_dm", "sometimes 2->1 like in LR-TDDFT".
_nk = kvec_d.size() / spin_mult is a separate assumption: it holds only when kvec_d is the full spin-doubled K_Vectors::kvec_d. The guard at :46-50 checks divisibility only, which is the weakest consequence of that precondition.
Four call sites now pass an nspin that the derivation cannot reconstruct the old values from:
lr_spectrum.cpp:16— for gamma-onlynspin 2,_nkbecomes 2. This is thedmr_gamma.cpp:19assertion that aborts 51/52/53/54. Past the assertion,spin_multalso collapses to 1, sodmrholds a single container while:60indexes.at(is)up tonspin_x - 1.exciton_plotter.cpp:481— same construction;:495then passesnspin_xas the channel count against a size-1get_dmr_vec().dftu_nao_op.cpp:165—kvec_d_fullis built at:159-163fromnk_local = get_nks()/nspin0spin-up k-points, so its length is already per-spin-channel; the old call passedkvec_d_full.size()explicitly. Fornspin 2the new_nkhalves, and the second half of the spin-up DMK is accumulated intoDMR[1]with the wrong k-phases — wrong Hubbard occupation matrices underdft_plus_u 1+symmetry 1+nspin 2. If the star count is odd, the:46-50guard aborts the run instead.update_state_rdmft.cpp:121—_nkgoes fromnks(old value clamped by the old constructor) tonks/2fornspin 2. The new value may well be the correct one, but it is a change.
Only (1) is visible to CI. 17_DS_DFTU is commented out in .github/workflows/test.yml:341-346 and also excluded from the Other Unittests catch-all at :353, so (3) produces no signal at all; 09_DeePKS and 10_others were skipped after the 08_EXX failure and are unverified on this branch. Making CI green is therefore necessary but not sufficient here.
Worth deciding explicitly whether DensityMatrix is the KS ground-state density matrix — in which case the derivation is correct and the LR / DFT+U / RDMFT users need a different entry point — or a general DM-shaped container over an arbitrary k-list, in which case spin_mult and nk have to stay caller-supplied.
Critsium-xy
left a comment
There was a problem hiding this comment.
Follow-up to the above, with one additional site and one correction.
dftu_nao_op.cpp:149,165 also loses the Pauli branch.
const int nspin0 = (this->nspin == 2) ? 2 : 1; // nspin == 4 -> nspin0 == 1
dmr_sym.reset(new module_dm::DensityMatrix<TK, double>(pv, kvec_d_full, nspin0));nspin0 is a spin_mult-shaped value, but the constructor stores its third argument as the physical nspin. For nspin 4 the symmetry-restored DM therefore carries dm.nspin == 1, and cal_dmr takes the real-projection branch at dmr_k.cpp:52 instead of the Pauli branch at :58. The old call was safe here because the branch condition was the global PARAM.inp.nspin == 4, independent of what the caller passed as _nspin.
This is the same error as dcad8913d, which 96e1e6d27 fixed at allocate_dm.cpp and edm.cpp but not here. tests/17_DS_DFTU/04_LCAO_DFTU_S4_XY (symmetry 1, nspin 4, dft_plus_u 1) is the matching case, and it produces no signal for the CI reason noted above. So at this site both _nk and the branch selection are wrong.
Correction to my previous comment. I led with the spin_mult = f(nspin) counterexample, but that is not what aborts CI. 51/52/53 are closed-shell, nspin_x == 1, so spin_mult happens to come out right; they die on _nk = kvec_d.size() / spin_mult giving 2 where the gamma path requires 1. The two assumptions fail independently:
| site | spin_mult wrong |
_nk wrong |
Pauli branch lost |
|---|---|---|---|
lr_spectrum.cpp:16 (closed-shell, 51/52/53) |
no | yes | no |
lr_spectrum.cpp:16 (open-shell, 54) |
yes | yes | no |
exciton_plotter.cpp:481 |
yes | yes | no |
dftu_nao_op.cpp:165 |
no | yes | yes (nspin 4) |
update_state_rdmft.cpp:121 |
no | yes | no |
Restoring only nk still leaves 54 with spin_mult == 1; restoring only spin_mult still leaves 51/52/53 and DFT+U broken.
The DFT+U case also shows the parameter is carrying two meanings at once -- "physical nspin, which selects the Pauli branch" and "the number that determines spin_mult" -- which is what let a spin_mult-shaped value be passed in without a compiler or reviewer noticing. Splitting them may be worth considering regardless of how the nk derivation is resolved.
Static reading only; I have not run the DFT+U cases.
AsTonyshment
left a comment
There was a problem hiding this comment.
Some correctness issues need to be resolved.
The LR and DFT+U correctness issues identified by @Critsium-xy are confirmed. SOC current output has an additional representation mismatch: Pauli density components are contracted with an untransformed spin-matrix velocity operator, producing incorrect currents. The LR failure also occurs during the excitation solve, and the complex open-shell spectrum can silently double-count the transition density.
b0498bb to
c525ffb
Compare
'T' and 'C' give identical operand dimensions; 'C' additionally conjugates the transposed operand. The previous comment incorrectly claimed 'C' would change the GEMM dimension.
This is a local analysis output file that should not be tracked.
AsTonyshment
left a comment
There was a problem hiding this comment.
LGTM now if CI could pass.
Critsium-xy
left a comment
There was a problem hiding this comment.
Two items on the current head, both small.
1. const int nspin = 0 default on the DensityMatrix constructors
cal_dmr now selects the SOC branch from the member dm.nspin (dmr_k.cpp:52,58), where develop used the global PARAM.inp.nspin. That member comes from a defaulted parameter:
// density_matrix.h:198, :206
DensityMatrix(..., const int nk, const int nspin = 0);
DensityMatrix(const Parallel_Orbitals* pv, const int spin_mult, const int nspin = 0);
// density_matrix.cpp:40
nspin(nspin > 0 ? nspin : spin_mult)Only two sites pass it: allocate_dm.cpp:22 and edm.cpp:69. Four other multi-k sites that go on to call cal_dmr / cal_dmr_td do not, so nspin silently degrades to spin_mult, which is 1 under SOC:
source_io/module_dos/cal_ldos.cpp:54(nspin_dm)source_io/module_chgpot/get_pchg_lcao.cpp:150(nspin_dm)source_lcao/module_dftu/dftu_nao_op.cpp:165(nspin0)source_io/module_current/td_current_io.cpp:57,:226(nspin_dm)
At nspin 4 these take the real-projection branch instead of the Pauli branch, while cal_gint_rho(..., 4, ...) still reads four Pauli components — rho_x/y/z come out empty. No assertion, no crash.
CI cannot see any of it:
- LCAO +
nspin 4+out_pchg: no case exists. out_ldos: the only case istests/01_PW/scf_out_ldos, PW basis,nspindefault 1.out_current: all ten cases arenspindefault 1.- DFT+U:
tests/17_DS_DFTU/04_LCAO_DFTU_S4_XYis exactlysymmetry 1+nspin 4+dft_plus_u 1+ LCAO, but17_DS_DFTUis commented out at.github/workflows/test.yml:341-346. The one DFT+U +nspin 4case that does run,03_NAO_multik/scf_u_spin4, setslspinorb 1, sosymmetryresolves to-1(read_inp_sys.cpp:290) and thedmr_symbranch atdftu_nao_op.cpp:138is never entered.
dftu_nao_op.cpp:165 is the site @Critsium-xy flagged at 06:34; the rollback fixed _nk there but not the branch selection.
Suggested fix: drop the default and pass the value explicitly at the four sites. The compiler then enumerates anything that was missed, which is the property the default removes. It also keeps 44947ff17 ("remove default parameters (Phase 1f)") and the no-new-default-arguments rule in AGENTS.md consistent with the rest of the PR.
For td_current_io the representation question is already deferred to a later PR. The non-Pauli branch may well be what that consumer wants, but it is currently reached through a default rather than a decision — passing nspin_dm explicitly with a one-line comment would record the intent either way.
2. Init_DM_Config member initializers
dm_routine.h:24-26:
int td_stype;
int nspin;
double nelec;
const std::map<...>* td_phase_hybrid = nullptr; // only this one is initializedThe struct is default-constructed and then filled field by field (esolver_ks_lcao.cpp:395-406), so a field added later but not assigned at the call site is an indeterminate read rather than a compile error. Worth giving all three a default member initializer.
Related: dm_routine.cpp:28 dereferences *cfg.td_phase_hybrid, and the non-null contract (td_stype == 2 && esolver_type != "tddft") is written out separately at the call site and in the implementation. An assert would make that contract local.
very good suggestions, I will refactor these two places with a more systematic way in the future |
Upstream deepmodeling#8000 renamed elecstate::DensityMatrix to module_dm::DensityMatrix and get_DMR_vector/get_DMR_pointer to get_dmr_vec/get_dmr_ptr; update the spin-index overload added in this branch accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
.editorconfig has required `indent_style = space` with `indent_size = 4`
for the whole repository, but 339 files under source/ still indent with
tabs. This converts the leading whitespace of 253 of them.
Scope. Three groups were deliberately left out:
- 48 files touched by the pull requests open at the time of writing
(deepmodeling#8000, deepmodeling#7924, deepmodeling#7906, deepmodeling#8005 and others), so this does not force a
conflict on work in flight;
- 5 vendored files: source_base/libm/ is ported from glibc-2.36 and
carries its own LICENCE, and source_base/mcd.c is Softpixel
MemCheckDeluxe under a BSD licence. Reformatting vendored sources
makes future syncs with their upstream harder;
- tabs that appear after the first non-blank character (alignment
tabs, 1193 lines). Only leading indentation is converted.
Method and verification. `expand -i -t4`, which rewrites the initial
whitespace of a line and nothing else, followed by three checks:
- `git diff -w --stat` is empty, so not one non-whitespace character
changed anywhere in the diff;
- of the 42 raw string literals in the changed files (222 lines, all
in source_io/module_parameter/read_inp_out.cpp), none has a line
that this commit touches. Leading whitespace inside `R"(...)"` is
part of the string, so that was the one place a leading-whitespace
rewrite could have changed behaviour;
- no changed line follows a line ending in a backslash, so no
backslash-continued string literal is affected either;
- no file gained a CR.
Effect on tools/03_code_analysis/code_quality_score.py: average score
over source/ goes from 82.15 to 82.58 and the number of passing files
from 1560 to 1562.
Six files score 1 to 3 points lower, all through the `line_too_long`
rule, because the scorer counts a tab as a single character while it
renders as up to four columns. The lines were already over 120 columns
on screen; the tab was hiding it. One of them,
module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just
below the tool's pass line. Wrapping those lines would mean editing code
in a commit that is otherwise whitespace-only, so it is left for a
follow-up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Fix spin-channel mismatch in LR complex transition dipole (length gauge) LR_Spectrum<complex<double>>::cal_transition_dipole_istate_length looped over all nspin_x spin channels of the transition density matrix but called get_DMR_real_imag_part() without selecting a channel, so the function copied every channel of DM_trans (size nspin_x) into the single-channel DM_trans_real_imag. For nspin_x==2 (open-shell/spin- polarized, multi-k) this hits an assertion failure (or out-of-bounds access in release builds), and even when it doesn't crash it double- counts the merged density on each iteration of the outer loop. Add an overload of get_DMR_real_imag_part() that copies a single named spin channel into the (always single-channel) DMR_real, and use it from the two call sites in cal_transition_dipole_istate_length so each outer loop iteration only processes its own channel, matching the double (gamma-only) specialization's behavior. Verified: reproduced the original assertion failure with gdb on the open-shell (nspin=2) multi-k length-gauge path, confirmed the fix resolves it, and confirmed the fixed multi-k oscillator strength / transition dipoles match the gamma-only result exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix segfault in LR real/imag HContainer helpers under MPI-parallel runs get_DMR_real_imag_part() (both overloads) and set_HR_real_imag_part() dereferenced the result of HContainer::find_pair(ia, ja) without a null check. Under real multi-rank MPI parallelism, the HContainer is distributed 2D block-cyclic, so find_pair() legitimately returns nullptr for atom pairs not owned by the current rank — dereferencing that pointer segfaults. Reproduced with gdb on tests/08_EXX/54_GO_ULR_HF (gamma_only=0, KPT 1 1 1, mpirun -np 2): SIGSEGV in get_DMR_real_imag_part, called from OperatorLRHxc::grid_calculation during the Casida eigenvalue solve — a different call site/crash than the spin-channel-mismatch bug fixed in the previous commit, and only reproducible with more than one MPI rank (a single-rank run owns every atom pair locally, masking the bug). Skip atom pairs not present on the local rank, matching the existing pattern used elsewhere in the codebase for MPI-parallel HContainer access. Verified: the same 54_GO_ULR_HF case now runs cleanly under mpirun -np 2 (gdb shows a clean exit, no crash) and its excitation energies exactly match both the gamma_only=1 run and result.ref. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Adapt new get_DMR_real_imag_part overload to module_dm refactor Upstream #8000 renamed elecstate::DensityMatrix to module_dm::DensityMatrix and get_DMR_vector/get_DMR_pointer to get_dmr_vec/get_dmr_ptr; update the spin-index overload added in this branch accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
.editorconfig has required `indent_style = space` with `indent_size = 4`
for the whole repository, but 339 files under source/ still indent with
tabs. This converts the leading whitespace of 253 of them.
Scope. Three groups were deliberately left out:
- 48 files touched by the pull requests open at the time of writing
(deepmodeling#8000, deepmodeling#7924, deepmodeling#7906, deepmodeling#8005 and others), so this does not force a
conflict on work in flight;
- 5 vendored files: source_base/libm/ is ported from glibc-2.36 and
carries its own LICENCE, and source_base/mcd.c is Softpixel
MemCheckDeluxe under a BSD licence. Reformatting vendored sources
makes future syncs with their upstream harder;
- tabs that appear after the first non-blank character (alignment
tabs, 1193 lines). Only leading indentation is converted.
Method and verification. `expand -i -t4`, which rewrites the initial
whitespace of a line and nothing else, followed by three checks:
- `git diff -w --stat` is empty, so not one non-whitespace character
changed anywhere in the diff;
- of the 42 raw string literals in the changed files (222 lines, all
in source_io/module_parameter/read_inp_out.cpp), none has a line
that this commit touches. Leading whitespace inside `R"(...)"` is
part of the string, so that was the one place a leading-whitespace
rewrite could have changed behaviour;
- no changed line follows a line ending in a backslash, so no
backslash-continued string literal is affected either;
- no file gained a CR.
Effect on tools/03_code_analysis/code_quality_score.py: average score
over source/ goes from 82.15 to 82.58 and the number of passing files
from 1560 to 1562.
Six files score 1 to 3 points lower, all through the `line_too_long`
rule, because the scorer counts a tab as a single character while it
renders as up to four columns. The lines were already over 120 columns
on screen; the tab was hiding it. One of them,
module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just
below the tool's pass line. Wrapping those lines would mean editing code
in a commit that is otherwise whitespace-only, so it is left for a
follow-up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.editorconfig has required `indent_style = space` with `indent_size = 4` for the whole repository, but 339 files under source/ still indent with tabs. This converts the leading whitespace of 252 of them (253 when prepared; source_lcao/setup_dm.h has since been deleted upstream by deepmodeling#8000). Scope. Three groups were deliberately left out: - 48 files touched by the pull requests open at the time of writing (deepmodeling#8000, deepmodeling#7924, deepmodeling#7906, deepmodeling#8005 and others), so this does not force a conflict on work in flight; - 5 vendored files: source_base/libm/ is ported from glibc-2.36 and carries its own LICENCE, and source_base/mcd.c is Softpixel MemCheckDeluxe under a BSD licence. Reformatting vendored sources makes future syncs with their upstream harder; - tabs that appear after the first non-blank character (alignment tabs, 1193 lines). Only leading indentation is converted. Method and verification. `expand -i -t4`, which rewrites the initial whitespace of a line and nothing else, followed by three checks: - `git diff -w --stat` is empty, so not one non-whitespace character changed anywhere in the diff; - of the 42 raw string literals in the changed files (222 lines, all in source_io/module_parameter/read_inp_out.cpp), none has a line that this commit touches. Leading whitespace inside `R"(...)"` is part of the string, so that was the one place a leading-whitespace rewrite could have changed behaviour; - no changed line follows a line ending in a backslash, so no backslash-continued string literal is affected either; - no file gained a CR. Effect on tools/03_code_analysis/code_quality_score.py: average score over source/ goes from 82.15 to 82.58 and the number of passing files from 1560 to 1562. Six files score 1 to 3 points lower, all through the `line_too_long` rule, because the scorer counts a tab as a single character while it renders as up to four columns. The lines were already over 120 columns on screen; the tab was hiding it. One of them, module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just below the tool's pass line. Wrapping those lines would mean editing code in a commit that is otherwise whitespace-only, so it is left for a follow-up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
.editorconfig has required `indent_style = space` with `indent_size = 4` for the whole repository, but 339 files under source/ still indent with tabs. This converts the leading whitespace of 248 of them. 253 files were selected when this was prepared. Since then, develop has deleted source_lcao/setup_dm.h (deepmodeling#8000) and source_pw/module_stodft/hamilt_sdft_pw.cpp (deepmodeling#8012), and already converted source_base/module_out/binstream.{h,cpp} (deepmodeling#8025) and source_pw/module_stodft/sto_hamilt_pw.h (renamed from hamilt_sdft_pw.h in deepmodeling#8012), which leaves 248. The conversion follows files that develop moved, e.g. onsite_proj_tools_stress.cpp is now under module_proj/ (deepmodeling#8007). Scope. Three groups were deliberately left out: - 48 files touched by the pull requests open at the time of writing (deepmodeling#8000, deepmodeling#7924, deepmodeling#7906, deepmodeling#8005 and others), so this does not force a conflict on work in flight; - 5 vendored files: source_base/libm/ is ported from glibc-2.36 and carries its own LICENCE, and source_base/mcd.c is Softpixel MemCheckDeluxe under a BSD licence. Reformatting vendored sources makes future syncs with their upstream harder; - tabs that appear after the first non-blank character (alignment tabs, 1193 lines). Only leading indentation is converted. Method and verification. `expand -i -t4`, which rewrites the initial whitespace of a line and nothing else, followed by three checks: - `git diff -w --stat` is empty, so not one non-whitespace character changed anywhere in the diff; - of the 42 raw string literals in the changed files (222 lines, all in source_io/module_parameter/read_inp_out.cpp), none has a line that this commit touches. Leading whitespace inside `R"(...)"` is part of the string, so that was the one place a leading-whitespace rewrite could have changed behaviour; - no changed line follows a line ending in a backslash, so no backslash-continued string literal is affected either; - no file gained a CR. Effect on tools/03_code_analysis/code_quality_score.py: average score over source/ goes from 82.15 to 82.58 and the number of passing files from 1560 to 1562. Six files score 1 to 3 points lower, all through the `line_too_long` rule, because the scorer counts a tab as a single character while it renders as up to four columns. The lines were already over 120 columns on screen; the tab was hiding it. One of them, module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just below the tool's pass line. Wrapping those lines would mean editing code in a commit that is otherwise whitespace-only, so it is left for a follow-up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
.editorconfig has required `indent_style = space` with `indent_size = 4` for the whole repository, but 339 files under source/ still indent with tabs. This converts the leading whitespace of 246 of them. 253 files were selected when this was prepared. Since then, develop has deleted two of them and already converted five others, which leaves 246: - deleted: source_lcao/setup_dm.h (deepmodeling#8000), source_pw/module_stodft/hamilt_sdft_pw.cpp (deepmodeling#8012); - already converted: source_base/module_out/binstream.{h,cpp} (deepmodeling#8025), source_pw/module_stodft/sto_hamilt_pw.h (renamed from hamilt_sdft_pw.h in deepmodeling#8012), source_io/module_ctrl/ctrl_output_pw.h and source_pw/module_pwdft/op_pw_nl.cpp (deepmodeling#8043). Files that develop moved are converted at their new path, e.g. onsite_proj_tools_stress.cpp is now under source_pw/module_proj/ (deepmodeling#8007). Scope. Three groups were deliberately left out: - 48 files touched by the pull requests open at the time of writing (deepmodeling#8000, deepmodeling#7924, deepmodeling#7906, deepmodeling#8005 and others), so this does not force a conflict on work in flight; - 5 vendored files: source_base/libm/ is ported from glibc-2.36 and carries its own LICENCE, and source_base/mcd.c is Softpixel MemCheckDeluxe under a BSD licence. Reformatting vendored sources makes future syncs with their upstream harder; - tabs that appear after the first non-blank character (alignment tabs, 1193 lines). Only leading indentation is converted. Method and verification. `expand -i -t4`, which rewrites the initial whitespace of a line and nothing else, followed by three checks: - `git diff -w --stat` is empty, so not one non-whitespace character changed anywhere in the diff; - of the 42 raw string literals in the changed files (222 lines, all in source_io/module_parameter/read_inp_out.cpp), none has a line that this commit touches. Leading whitespace inside `R"(...)"` is part of the string, so that was the one place a leading-whitespace rewrite could have changed behaviour; - no changed line follows a line ending in a backslash, so no backslash-continued string literal is affected either; - no file gained a CR. Effect on tools/03_code_analysis/code_quality_score.py: average score over source/ goes from 82.15 to 82.58 and the number of passing files from 1560 to 1562. Six files score 1 to 3 points lower, all through the `line_too_long` rule, because the scorer counts a tab as a single character while it renders as up to four columns. The lines were already over 120 columns on screen; the tab was hiding it. One of them, module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just below the tool's pass line. Wrapping those lines would mean editing code in a commit that is otherwise whitespace-only, so it is left for a follow-up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…o spaces (#8011) * docs: fix two @file tags that name a different file Doxygen's @file takes the name of the file it documents. These two name a file that does not exist, so Doxygen attributes the block to the wrong (or to no) file: source_base/ndarray.h said NDArray.h source_lcao/module_rt/band_energy.h said bandenegy.h (also a typo) Found by tools/03_code_analysis/code_quality_score.py (rule doc_file_mismatch). The scan also flagged source_pw/module_pwdft/radial_proj.h (said radial_projection.h), but #8007 has since moved that header to source_pw/module_proj/ with the tag already corrected, so it is no longer part of this commit. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * style: convert leading tab indentation to spaces in 246 source files .editorconfig has required `indent_style = space` with `indent_size = 4` for the whole repository, but 339 files under source/ still indent with tabs. This converts the leading whitespace of 246 of them. 253 files were selected when this was prepared. Since then, develop has deleted two of them and already converted five others, which leaves 246: - deleted: source_lcao/setup_dm.h (#8000), source_pw/module_stodft/hamilt_sdft_pw.cpp (#8012); - already converted: source_base/module_out/binstream.{h,cpp} (#8025), source_pw/module_stodft/sto_hamilt_pw.h (renamed from hamilt_sdft_pw.h in #8012), source_io/module_ctrl/ctrl_output_pw.h and source_pw/module_pwdft/op_pw_nl.cpp (#8043). Files that develop moved are converted at their new path, e.g. onsite_proj_tools_stress.cpp is now under source_pw/module_proj/ (#8007). Scope. Three groups were deliberately left out: - 48 files touched by the pull requests open at the time of writing (#8000, #7924, #7906, #8005 and others), so this does not force a conflict on work in flight; - 5 vendored files: source_base/libm/ is ported from glibc-2.36 and carries its own LICENCE, and source_base/mcd.c is Softpixel MemCheckDeluxe under a BSD licence. Reformatting vendored sources makes future syncs with their upstream harder; - tabs that appear after the first non-blank character (alignment tabs, 1193 lines). Only leading indentation is converted. Method and verification. `expand -i -t4`, which rewrites the initial whitespace of a line and nothing else, followed by three checks: - `git diff -w --stat` is empty, so not one non-whitespace character changed anywhere in the diff; - of the 42 raw string literals in the changed files (222 lines, all in source_io/module_parameter/read_inp_out.cpp), none has a line that this commit touches. Leading whitespace inside `R"(...)"` is part of the string, so that was the one place a leading-whitespace rewrite could have changed behaviour; - no changed line follows a line ending in a backslash, so no backslash-continued string literal is affected either; - no file gained a CR. Effect on tools/03_code_analysis/code_quality_score.py: average score over source/ goes from 82.15 to 82.58 and the number of passing files from 1560 to 1562. Six files score 1 to 3 points lower, all through the `line_too_long` rule, because the scorer counts a tab as a single character while it renders as up to four columns. The lines were already over 120 columns on screen; the tab was hiding it. One of them, module_ri/exx_abfs_ctor_orbs.cpp, moves from 60 to 59 and so drops just below the tool's pass line. Wrapping those lines would mean editing code in a commit that is otherwise whitespace-only, so it is left for a follow-up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Summary of the module_dm refactoring (from 5161a8b)
This PR carries out a staged cleanup and restructuring of the density-matrix module (module_dm), in preparation for future work on density-matrix-based algorithms. The refactoring is behavior-preserving and is organized into several phases:
Style and hygiene (Phase 1): Removed duplicate doc-block comments, replaced auto with explicit types, unindented preprocessor directives, wrapped lines over 120 characters, converted tab indentation to spaces, and removed default arguments from interfaces (call sites now pass values explicitly).
Memory safety (Phase 2): Replaced raw new/delete with std::unique_ptr in density_matrix_io and with std::vector in cal_edm_tddft and for the dmr_tmp_ buffer, eliminating manual memory management.
Decoupling from globals (Phase 3): Eliminated direct PARAM/global dependencies in density_matrix.cpp, init_dm, and cal_edm_tddft; configuration values (e.g., nlocal) are now passed in explicitly.
File and namespace restructuring (Phase 4): Split the monolithic density_matrix.cpp/dmr_cal.cpp into focused units (dmr_gamma, dmr_k, dmr_td, dmr_full), split cal_edm_tddft into edm_tddft + edm_tddft_lapack, moved DensityMatrix and helpers from elecstate into the module_dm namespace, removed the reverse dependency of module_dm on source_lcao, moved Setup_DM/allocate_dm and Record_adj to their proper homes (module_dm and source_cell), and extracted read_DMK/write_DMK as free functions instead of thin member wrappers.
Naming consistency: Renamed files, classes, member functions, and free functions to a uniform snake_case scheme (_DMR/_DMK/EDMK/pexsi_EDM → dmr/dmk/edmk/edm_pexsi, cal_dm_psi/cal_dmk_psi → dm_from_psi/dmk_from_psi, _paraV → pv, DensityMatrix_Tools namespace flattened into module_dm), and updated comments and reference logs accordingly.
Code quality and correctness along the way: Unified the psi-to-density-matrix kernel into a single module_dm::cal_dmk_psi, deduplicated the k-point/TD DMR loops into shared helpers with formula comments, paired ModuleBase::timer start/end at function scope, removed dead code (unused get_kvec_d, read_DMK_file/write_DMK_file, redundant ZEROS calls), moved unit tests from test/ to unittests/, and fixed SOC/noncollinear issues (Pauli branch selected by global nspin, global matrix dimension in the TDDFT EDM, and DFT+U test/CMake link fixes).