Skip to content

Split some large files (fs_nonlocal_tools.cpp, onsite_proj.cpp, to_wannier90_lcao.cpp, etc.) into smaller ones - #7873

Merged
mohanchen merged 46 commits into
deepmodeling:developfrom
mohanchen:2026-08-28-2
Aug 31, 2026
Merged

Split some large files (fs_nonlocal_tools.cpp, onsite_proj.cpp, to_wannier90_lcao.cpp, etc.) into smaller ones#7873
mohanchen merged 46 commits into
deepmodeling:developfrom
mohanchen:2026-08-28-2

Conversation

@mohanchen

@mohanchen mohanchen commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Split some large files into smaller ones

Refactor xc_grad pipeline: pack stage arguments into internal structs

The gradcorr pipeline stages (gradcorr_prepare_rho / gradcorr_xc_kernel /
gradcorr_assemble_vxc) took 20+ mostly same-typed scalar parameters
(pointer/array pairs for per-spin buffers, plus several int/bool flags
with fixed ordering conventions). This made silent argument swaps easy
to introduce and hard to review.

Move the three stage functions out of the XC_Functional class into
internal free functions declared in a new xc_grad_internal.h, which is
included only by the xc_grad*.cpp implementation files. Their parameters
are packed into two structs:

  • GradCorrParams: read-only inputs (charge, basis, ucell, spin flags,
    hybrid parameters, functional metadata), built once by
    XC_Functional::gradcorr and passed by const reference;
  • GradCorrBuffers: per-spin scratch arrays (rhotmp/rhogsum/gdr/h/lapl/
    vlapl/vsave/vgg/neg) allocated by the prepare stage and consumed by
    the kernel and assemble stages.

The static-members use_libxc/func_type/func_id, which the stage
functions previously read implicitly across translation units, are now
explicit fields of GradCorrParams. The public entry point
XC_Functional::gradcorr keeps its exact signature, so no caller changes.
Also drop the unused leftover epsg constant and need_laplacian unpacking
in the kernel stage.

Verification: xc_ target builds (ENABLE_LIBXC=OFF, gnu++14 + OpenMP);
g++ -fsyntax-only -D__LIBXC over the xc_grad*.cpp files covers the libxc
branch; nm confirms XC_Functional::gradcorr signature unchanged and a
single definition of each stage function; MODULE_ESTATE_elecstate_pw
builds, links and passes under OMP_NUM_THREADS=1; agent governance
check reports no findings.

This PR also renames the wannier90 module to w90: all files, classes and
references under source/source_io/module_wannier are shortened from
to_wannier90_* to to_w90_* (renames done with git mv to preserve file
history; CMakeLists.txt and Makefile.Objects updated accordingly).

On top of the rename, the touched code is hardened:

  • Replace raw new/delete with std::vector/std::unique_ptr across the
    module (toW90 member arrays, local buffers in pw/lcao setup/io/overlap,
    FR_overlap Leb_grid/FR_container, unk_inLcao and psi_initer_ ownership
    in lcao_pw), removing all manual delete[] cleanup.
  • Fix naming (MGT -> mgt_, NULL -> nullptr), tab indentation and
    overlong lines.
  • Remove global dependencies: nspin, nbands, nqx, dq and npol are
    now passed explicitly through the toW90 constructor chain, so the module
    no longer reads PARAM.inp.* / PARAM.globalv.* for these.
  • Drop 12 redundant #ifdef __MPI guards around
    Parallel_Reduce::reduce_all calls. To make serial builds safe for
    broadcast as well, parallel_common.cpp is restructured so every
    bcast_* function keeps its #ifdef __MPI inside the function body
    (no-op definitions when built without MPI, following the
    parallel_reduce.cpp style). This also fixes the "build without MPI"
    link failure reported by CI (undefined reference to
    Parallel_Common::bcast_bool).

Verification: every step was committed only after a clean incremental build;
g++ -fsyntax-only of parallel_common.cpp passes without __MPI; the
code-quality score of the changed files improved from 76.2 to 78.8 with
13/16 files passing (remaining deductions are pre-existing global-dependency
and cyclomatic-complexity debt, proposed for follow-up PRs). The branch was
rebased onto the latest develop (DFT+U step 5); 13 zero-net-effect
split+revert commits were dropped during the rebase to keep the history
linear, with tree content verified identical before and after.

@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Aug 28, 2026
@mohanchen
mohanchen requested a review from Critsium-xy August 28, 2026 08:13
@mohanchen mohanchen changed the title Split some large files into smaller ones Split some large files (fs_nonlocal_tools.cpp and onsite_proj.cpp) into smaller ones Aug 28, 2026
@mohanchen mohanchen changed the title Split some large files (fs_nonlocal_tools.cpp and onsite_proj.cpp) into smaller ones Split some large files (fs_nonlocal_tools.cpp, onsite_proj.cpp, to_wannier90_lcao.cpp) into smaller ones Aug 28, 2026
@mohanchen mohanchen changed the title Split some large files (fs_nonlocal_tools.cpp, onsite_proj.cpp, to_wannier90_lcao.cpp) into smaller ones Split some large files (fs_nonlocal_tools.cpp, onsite_proj.cpp, to_wannier90_lcao.cpp, etc.) into smaller ones Aug 28, 2026

@Critsium-xy Critsium-xy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled this PR locally and mechanically checked each of the 12 split groups: for every group I stripped comments, normalized whitespace, dropped #include lines, and sorted the content of the original file and of the union of the resulting files, then diffed them to see whether any code was actually added or removed. The splits are faithful overall — 11 of the 12 groups are token-for-token equivalent to the original, with the only differences being duplicated includes / #ifdef guards and template instantiation lines. Three things worth raising.

1. Duplicated whole-class explicit template instantiations

After the split, the same specialization is explicitly instantiated in multiple TUs:

  • atom_pair.cpp / atom_pair_kernels.cpp / atom_pair_output.cpp: each file carries 3 lines of template class AtomPair<float/double/std::complex<double>>;
  • hcontainer.cpp / hcontainer_insert.cpp / hcontainer_math.cpp: likewise 3 lines each of template class HContainer<...>;
  • force_pw.cpp / force_pw_driver.cpp / force_pw_ew.cpp: 2 lines each of template class Forces<double, DEVICE_CPU/DEVICE_GPU>;
  • force_stress_lcao_driver.cpp / _integral.cpp / _pw.cpp / _utility.cpp: 2 lines each of template class Force_Stress_LCAO<double/std::complex<double>>;

Per [temp.explicit]/13, a given specialization may have at most one explicit instantiation definition in a program; violating this is IFNDR. GCC/Clang link it fine via COMDAT weak symbols, which is why CI is green — but the practical effect is that every TU re-instantiates all members visible from the headers, inflating object size and link time. That works against the compile-time speedup the split is meant to deliver.

onsite_proj.cpp and onsite_proj_tools.cpp in this same PR use per-member instantiation (template void X<...>::f(...);), which is the correct form. I checked all 12 out-of-line members of OnsiteProjector (including get_instance and the destructor) and none are missing. Suggest switching the hcontainer / force groups to per-member instantiation as well.

2. force_stress_lcao lost 118 lines of explanatory comments

While moving code from force_stress_lcao.cpp into force_stress_lcao_driver.cpp and friends, 118 comment lines were not carried over. These are not commented-out dead code — they carry real information, e.g.:

// The legacy dft_plus_u==2 force/stress path is currently broken.
// With symmetry switched on, the forces assembled above are built from IBZ-reduced ...
// Force symmetrization is linear, so it commutes with the removal of a ...
// Force contribution from DFT+U, Quxin add on 20201029
// Background: Plus_U::force_stress relies on ForceStressArrays

I confirmed with git grep that these no longer exist anywhere on the PR branch. The first one is the most costly to lose — it flags a known defect. Suggest restoring this batch of comments.

Comment loss in the other groups is minor (11 lines in force_pw, 15 in numerical_basis) and is almost entirely commented-out dead code, which is fine to drop.

3. xc_grad is not a pure split — worth calling out in the PR description

Beyond splitting files, the xc_grad group contains a substantive refactor: raw new[]/delete[] pointers were converted to std::vector (removing 20 delete[] calls), and double** vgg / double** vsave became flat arrays, changing indexing from vgg[is][ir] to vgg[is*nrxx+ir].

I checked the prepare / kernel / assemble sections one by one and the semantics are equivalent: vgg is allocated as nspin0*nrxx and vsave as nspin*nrxx, matching the indexing; the 310-line kernel section is a verbatim move (diff -w shows only the function header/footer); and passing vtxcgc by value, accumulating into it, then doing vtxc += vtxcgc behaves the same as the original local variable. The change itself is a good direction.

But the PR description currently only says "Split some large files into smaller ones", so a reader will assume this is a purely mechanical move and skip the xc_grad diff. Suggest stating explicitly in the description that this group also rewrites memory management and indexing, or splitting it into its own PR to keep future bisects clean.

Separately, the three new public static functions added to xc_functional.h take 26 / 27 / 24 parameters, all bool, double and raw pointers — getting the argument order wrong would not be caught by the compiler. That header is also widely included, so touching it forces a full rebuild, which offsets the benefit of the split. Suggest packing the parameters into a struct and moving these declarations to an internal header (e.g. xc_grad_internal.h) rather than the public section of xc_functional.h.

abacus_fixer added 23 commits August 29, 2026 16:35
…te_proj_init.cpp

Step 1 of file split. Move init(), init_proj(), read_abacus_orb() from
onsite_proj.cpp to a new onsite_proj_init.cpp. Switch the main file
from whole-class 'template class' instantiation to method-level
explicit instantiation so the moved methods are emitted from
onsite_proj_init.cpp only. README comment block moved along with init.

CMakeLists.txt updated to compile the new file. Verified by
'make -j 30' in build_max_para_test (built abacus_max_para successfully).
…upations into onsite_proj_overlap.cpp

Step 2 of file split. Move the three overlap/occupation methods to
onsite_proj_overlap.cpp and emit them via method-level instantiation.
Slim the main onsite_proj.cpp includes (drop onsite_proj_print.h,
cell_tools.h, parallel_reduce.h, parameter.h, timer.h, <vector>) that
were only needed by the moved methods.

Verified by 'make -j 30' in build_max_para_test (built abacus_max_para).
…orce_stress.cpp

Step 3 of file split. Move the four cal_{force,stress}_onsite_{dftu,dspin}
wrappers to onsite_proj_force_stress.cpp. Main onsite_proj.cpp now
contains only the singleton accessor and destructor, with method-level
instantiation for those two. Drop the now-unused dftu_base.h and
<complex> includes from the main file.

Verified by 'make -j 30' in build_max_para_test (built abacus_max_para).
…tools_becp.cpp

Move the cal_becp method definition from onsite_proj_tools.cpp to a new
file onsite_proj_tools_becp.cpp with method-level explicit instantiation
for the CPU specialization. Add required includes (math_polyint.h,
math_ylmreal.h) for Nonlocal_maths dependencies. Update CMakeLists.txt.

Build verified: make -j 30 in build_max_para_test succeeds.
…roj_tools.cpp

Move cal_dbecp_s, cal_dbecp_f, save_vkb, revert_vkb, and transfer_gcar
method definitions from onsite_proj_tools.cpp to a new file
onsite_proj_tools_dbecp.cpp with method-level explicit instantiation
for the CPU specialization. Add required includes (math_polyint.h,
math_ylmreal.h, kernels/force_op.h for cal_vkb1_nl_op). Update CMakeLists.

Build verified: make -j 30 in build_max_para_test succeeds.
Move cal_force_dftu and cal_force_dspin to onsite_proj_tools_force.cpp,
and cal_stress_dftu and cal_stress_dspin to onsite_proj_tools_stress.cpp.
Both new files use method-level explicit instantiation for the CPU
specialization. Update CMakeLists.txt.

Build verified: make -j 30 in build_max_para_test succeeds.
Split the 827-line fs_nonlocal_tools.cpp into four functionally-cohesive
files:
- fs_nonlocal_tools.cpp (core): constructor, destructor, allocate_memory,
  delete_memory, whole-class template instantiation
- fs_nonlocal_tools_vkb.cpp: cal_vkb, cal_becp, reduce_pool_becp
- fs_nonlocal_tools_stress.cpp: cal_vkb_deri_s, cal_dbecp_s, cal_stress
- fs_nonlocal_tools_force.cpp: cal_vkb_deri_f, cal_dbecp_f, save_vkb,
  revert_vkb, transfer_gcar, cal_force

Each new file uses method-level explicit instantiation for the CPU
specialization. Update CMakeLists.txt.

Build verified: make -j 30 in build_max_para_test succeeds.
Tests verified: MODULE_PW_pw_test, MODULE_PW_pwdft_soc, onsite_op_test,
dftu_pw/core/operator, deltaspin_pw/core all pass.
Add a bullet to the Required Baseline section: for multi-step refactors
such as splitting a large .cpp into several files, build and commit after
each step rather than batching all changes before verification.
Split the 1202-line to_wannier90_lcao.cpp into four functionally-cohesive
files (non-template class, no explicit instantiation needed):
- to_wannier90_lcao.cpp (core): constructor, destructor, calculate
- to_wannier90_lcao_io.cpp: cal_Mmn, cal_Amn, out_unk (file output)
- to_wannier90_lcao_setup.cpp: initialize_orb_table, set_R_coor,
  count_delta_k, unkdotkb, produce_basis_orb, produce_trial_in_lcao,
  construct_overlap_table_project
- to_wannier90_lcao_overlap.cpp: cal_orbA_overlap_R, unkdotA

All files wrapped in #ifdef __LCAO. Update CMakeLists.txt.

Build verified: make -j 30 in build_max_para_test succeeds.
Split the 1051-line xc_grad.cpp into three files:
- xc_grad.cpp (gradcorr): the monolithic gradient correction method
  (single 798-line method, cannot be mechanically split further)
- xc_grad_wfc.cpp: grad_wfc template method + explicit instantiation
- xc_grad_utils.cpp: grad_rho, grad_dot, laplacian_rho, noncolin_rho

Update CMakeLists.txt for xc_ library and test targets that compile
xc_grad.cpp directly (test_xc3/5/7, surchem cal_vcav/cal_vel).

Build verified: make -j 30 in build_max_para_test succeeds, including
all test executables.
Extract the rho preparation phase (FFT, array allocation, gradient and
laplacian computation) from the 798-line gradcorr method into a separate
gradcorr_prepare_rho function (26 parameters, all by reference).

gradcorr shrinks from 826 to 657 lines. The orchestrator now declares
the shared arrays, calls prepare_rho, then proceeds to the XC kernel
and potential assembly (to be extracted in steps 2-3).

Update xc_functional.h, CMakeLists.txt, and test CMakeLists that
compile xc_grad.cpp directly.

Build verified: make -j 30 in build_max_para_test succeeds.
Extract the main XC computation loop (#pragma omp parallel + #ifdef __LIBXC
nspin branches + OpenMP reduction) from gradcorr into a separate
gradcorr_xc_kernel function (27 parameters, all by reference/pointer).

gradcorr shrinks from 657 to 355 lines. The orchestrator now:
1. Guards + setup
2. Calls prepare_rho (step 1)
3. Calls xc_kernel (this step)
4. [Laplacian stress + assembly — to be extracted in step 3]
5. Cleanup

Update xc_functional.h, CMakeLists.txt, and test CMakeLists.

Build verified: make -j 30 in build_max_para_test succeeds.
Extract the potential assembly phase (Laplacian stress contribution,
vxc from vtxcgc + dh + laplacian, noncolinear rotation back) from
gradcorr into a separate gradcorr_assemble_vxc function (24 parameters).

gradcorr is now a 183-line orchestrator that:
1. Guards + early returns
2. Setup (nspin0, fac, need_laplacian, stress_gga init)
3. Variable declarations
4. Calls prepare_rho (step 1)
5. Calls xc_kernel (step 2)
6. Calls assemble_vxc (this step)
7. Cleanup (delete arrays)

The original 798-line monolith is now split across:
- xc_grad.cpp (183 lines, orchestrator)
- xc_grad_prepare.cpp (222 lines, rho preparation)
- xc_grad_kernel.cpp (351 lines, XC computation loop)
- xc_grad_assemble.cpp (222 lines, potential assembly)

Build verified: make -j 30 in build_max_para_test succeeds.
- Replace 9 raw pointer allocations (rhotmp1/2, rhogsum1/2, gdr1/2, h1/2, neg) with std::vector, removing all manual delete[] in the orchestrator
- Flatten 2D arrays vsave/vgg (double**) into 1D vectors (vsave[is*nrxx+ir] index pattern), eliminating nested heap allocations
- prepare_rho now resizes vectors instead of new; kernel receives raw pointers via .data() (signature unchanged)
- assemble_vxc signature updated: double** vsave/vgg -> double* (flattened 1D)
- Build verified: make -j 30, 5 surchem tests passed
…iles

After splitting fs_nonlocal_tools.cpp and onsite_proj_tools.cpp into
multiple .cpp files with method-level explicit instantiation, only CPU
instantiations were added to the split files. The whole-class explicit
instantiation in the main file does not instantiate methods defined in
other translation units, causing undefined reference errors for
DEVICE_GPU specializations during GPU linking.

Add #if ((defined __CUDA) || (defined __ROCM)) guarded GPU method-level
explicit instantiations mirroring the existing CPU ones in:
- fs_nonlocal_tools_vkb.cpp (cal_vkb/cal_becp/reduce_pool_becp)
- fs_nonlocal_tools_stress.cpp (cal_vkb_deri_s/cal_dbecp_s/cal_stress)
- fs_nonlocal_tools_force.cpp (cal_vkb_deri_f/cal_dbecp_f/save_vkb/
  revert_vkb/transfer_gcar/cal_force)
- onsite_proj_tools_dbecp.cpp (cal_dbecp_s/cal_dbecp_f/save_vkb/
  revert_vkb/transfer_gcar)
- onsite_proj_tools_force.cpp (cal_force_dftu/cal_force_dspin)
- onsite_proj_tools_stress.cpp (cal_stress_dftu/cal_stress_dspin)

Verified with local GPU build (USE_CUDA=ON): abacus_pw_gpu links cleanly.
- xc_grad_assemble.cpp: replace 'double* dh = new double[]' with
  'std::vector<double> dh', pass .data() to grad_dot
- xc_grad_utils.cpp (grad_rho): replace gdrtmp raw pointer with
  std::vector<std::complex<double>>, use .data() for recip2real
- xc_grad_utils.cpp (grad_dot): replace aux and gaux raw pointers
  with std::vector<std::complex<double>>, use .data() for FFT calls
- xc_functional.cpp: minor comment typo fix (GGA, plane-wave)

All replacements are zero-overhead (vector uses contiguous memory
identical to new[]), exception-safe (auto-dealloc on scope exit), and
consistent with the previous RAII migration in xc_grad.cpp.

Verified: make -j 30 passes; ctest -R 'GRADCORR|VXC|PBE|SCAN|LAPL|HSE' 7/7 passed.
…al/get_trial/integral to to_wannier90_pw_setup.cpp
…o to_wannier90_pw_overlap.cpp, reduce original to 77 lines (ctor/dtor/calculate/set_tpiba)
abacus_fixer added 16 commits August 29, 2026 16:35
…/Sq/V+cal_flq+cal_ylm+cal_gpow to numerical_basis_overlap.cpp
…overlap_Q/Sq/V to numerical_basis_output.cpp
The gradcorr pipeline stages (gradcorr_prepare_rho / gradcorr_xc_kernel /
gradcorr_assemble_vxc) took 20+ mostly same-typed scalar parameters
(pointer/array pairs for per-spin buffers, plus several int/bool flags
with fixed ordering conventions). This made silent argument swaps easy
to introduce and hard to review.

Move the three stage functions out of the XC_Functional class into
internal free functions declared in a new xc_grad_internal.h, which is
included only by the xc_grad*.cpp implementation files. Their parameters
are packed into two structs:

- GradCorrParams: read-only inputs (charge, basis, ucell, spin flags,
  hybrid parameters, functional metadata), built once by
  XC_Functional::gradcorr and passed by const reference;
- GradCorrBuffers: per-spin scratch arrays (rhotmp/rhogsum/gdr/h/lapl/
  vlapl/vsave/vgg/neg) allocated by the prepare stage and consumed by
  the kernel and assemble stages.

The static-members use_libxc/func_type/func_id, which the stage
functions previously read implicitly across translation units, are now
explicit fields of GradCorrParams. The public entry point
XC_Functional::gradcorr keeps its exact signature, so no caller changes.
Also drop the unused leftover epsg constant and need_laplacian unpacking
in the kernel stage.

Verification: xc_ target builds (ENABLE_LIBXC=OFF, gnu++14 + OpenMP);
g++ -fsyntax-only -D__LIBXC over the xc_grad*.cpp files covers the libxc
branch; nm confirms XC_Functional::gradcorr signature unchanged and a
single definition of each stage function; MODULE_ESTATE_elecstate_pw
builds, links and passes under OMP_NUM_THREADS=1; agent governance
check reports no findings.
- git mv to_wannier90* -> to_w90* in source/source_io/module_wannier
- Rename classes toWannier90, toWannier90_PW, toWannier90_LCAO,
  toWannier90_LCAO_IN_PW to toW90, toW90_PW, toW90_LCAO, toW90_LCAO_IN_PW
- Update include guards TOWannier90*_H -> TO_W90*_H and all includes
- Update call sites in ctrl_scf_lcao.cpp / ctrl_output_pw.cpp and the
  developer comment in read_inp_postproc.cpp
- Update source_io/CMakeLists.txt and source/Makefile.Objects
- Keep INPUT parameter 'towannier90' and user-facing Wannier90 strings
  unchanged (they refer to the external Wannier90 code)

Verification:
- grep -rn "toWannier90|to_wannier90|TOWannier90" source/: no residue
- cmake --build build --target io_advanced io_input -j 16: passed
- cmake --build build --target abacus_basic_para -j 16: linked OK
- python3 tools/03_code_analysis/agent_governance_check.py --staged: no
  errors (warnings are rename-induced include-line false positives)
Replace raw new/delete member arrays (R_centre, L, m, rvalue, alfa,
spin_qaxis etc.) with std::vector and drop the manual destructor cleanup.
Rename local ORB_gaunt_table MGT to mgt_, replace raw new/delete arrays
with std::vector, and wrap overlong lines.
Replace raw new/delete grid buffers with std::vector, own Leb_grid and
FR_container via std::unique_ptr, keep C++11 compatibility (reset(new)
instead of std::make_unique), fix tab indentation and overlong lines.
Convert local new/delete buffers (gk, sf, orbital arrays, radial
integration buffers) to std::vector, drop leftover delete[] statements,
and wrap overlong lines.
Replace new/delete buffers (porter, zpiece) with std::vector, use
nullptr instead of NULL, and wrap overlong output lines.
Convert phase/psir FFT buffers to std::vector with data() at call
sites, drop manual delete[], and wrap the overlong line.
Return std::unique_ptr from get_unk_from_lcao instead of a raw owning
pointer, drop the caller-side delete, and wrap overlong lines.
Allocate psi_init_nao and the new Psi through local std::unique_ptr and
release() into the owning members so ownership transfer stays explicit.
- Add nspin parameter to toW90 constructor chain (toW90, toW90_PW,
  toW90_LCAO, toW90_LCAO_IN_PW) and store it in member nspin_,
  replacing all 19 PARAM.inp.nspin references in module_wannier;
  callers in ctrl_output_pw.cpp and ctrl_scf_lcao.cpp pass inp.nspin
- Remove unneeded #ifdef __MPI around the GlobalV::MY_RANK check in
  toW90::out_eig, consistent with other MY_RANK checks in the module
- Verified: incremental build passes with no errors and all touched
  TUs rebuilt; grep confirms no PARAM.inp.nspin left in module_wannier
- Add nbands/nqx/dq/npol parameters to the toW90 constructor chain
  and store them in members nbands_/nqx_/dq_/npol_, removing all
  PARAM.inp.nbands and PARAM.globalv.nqx/dq/npol references in
  module_wannier (to_w90, pw_setup, lcao_setup, lcao_overlap,
  lcao_pw); ctrl callers pass PARAM values at construction
- Verified: incremental build passes with no errors; grep confirms
  no PARAM.inp.nbands / PARAM.globalv.nqx/dq/npol left in the module;
  quality score now 13/16 files passing (avg 78.8), pw_setup and
  lcao_overlap cross the pass line
Remove #ifdef __MPI/#endif around 12 Parallel_Reduce::reduce_all and
Parallel_Common::bcast_bool calls in module_wannier: these wrappers
are declared unconditionally and behave as no-op serially, so calling
them without preprocessor guards is safe and consistent with the
governance rule of using guarded wrappers instead of direct MPI.
Guards around MPI_Barrier, ScalapackConnector::gemm and the pool
partition logic in out_unk are kept.
- Verified: incremental build passes with no errors; 5 guards remain
  in the module, all wrapping MPI/ScaLAPACK-specific symbols or logic
Restructure parallel_common.cpp following parallel_reduce.cpp style:
the file-level #ifdef around all 11 bcast_* definitions is replaced
by function-body guards, so serial builds get no-op definitions
instead of link errors. Callers no longer need #ifdef __MPI around
bcast calls (consistent with Parallel_Reduce::reduce_* usage); the
guard previously restored around bcast_bool in to_w90.cpp is removed
again as the net change there is zero.

This fixes the "build without MPI" link failure reported by CI:
undefined reference to Parallel_Common::bcast_bool(bool&).

- Verified: incremental build with MPI passes with no errors;
  g++ -fsyntax-only of parallel_common.cpp without __MPI passes
@Critsium-xy

Copy link
Copy Markdown
Collaborator

Second review pass. I ran a stronger verification than last time and found one build-level issue plus a few smaller ones.

Verification: the split is mechanically sound

My first pass only compared sorted line sets, which cannot rule out statements being reordered inside a function. This time I parsed both trees into {signature -> body} maps via brace matching (descending into namespace blocks) and compared bodies pairwise across all 12 split groups.

Result: no function lost, no function added, no statement reordering anywhere. Only four bodies differ at all:

  • Force_Stress_LCAO<T>::getForceStress — whitespace only (isforce, isstress, false ); -> false);)
  • OnsiteProjector<T, Device>::init — whitespace only (for(int it=0;it<ntype;++it) -> for(int it=0; it<ntype; ++it))
  • Numerical_Basis::output_overlap_V — a stray empty statement ; removed
  • the xc_grad family — the RAII refactor noted in my previous review

Onsite_Proj_tools::cal_becp moved from inside namespace hamilt { } to a qualified hamilt:: definition; the body is byte-identical.

I also checked grad_rho and grad_dot individually since they are part of the xc_grad conversion: gdrtmp is still nmaxgr, aux is still nmaxgr, gaux is still npw — buffer sizes and call arguments all line up, so the conversion is faithful.

One more thing that could have gone wrong but did not: neither the original files nor the new ones contain static free functions or anonymous namespaces, so no previously TU-local helper was silently promoted to external linkage.

1. source/Makefile.Objects was not updated — the Makefile build will not link

None of the 36 new .cpp files are registered there, while source/Makefile includes it at line 195 and builds FP_OBJS from it at line 217. Missing entries mean undefined references at link time.

This file is actively maintained, not abandoned — on develop it covers essentially every .cpp in the directories this PR touches:

directory unregistered / total .cpp on develop
source_lcao 0 / 31
module_xc 1 / 17
module_hcontainer 0 / 8
module_symmetry 1 / 12
module_wannier 0 / 5
module_bessel 0 / 4
module_pwdft 1 / 60

Of the three exceptions, deltaspin_pw_impl.cpp is #included into other .cpp files rather than compiled standalone.

Worth flagging separately: the Build with Makefile & Intel compilers job would normally catch this, but its Build step takes 0 seconds (13:53:17 -> 13:53:17 on run 33177252527). I checked the last three runs of that workflow on develop and the Build step is 0 seconds there too, going back to at least 2026-07-20. So that job has been a no-op since before this PR — a pre-existing problem, but it means nothing will stop this from landing broken.

2. Include blocks were copied wholesale into every new TU

Across the 12 groups, #include lines went from 144 to 496 (3.4x), because each new file carries a verbatim copy of the original include block regardless of what it needs:

  • All six xc_grad TUs carry the same 11-line block. xc_grad_utils.cpp uses neither ATen nor libxc (0 references once the use_libxc static member is excluded), yet pulls in <ATen/core/tensor.h>, xc_functional_op.h, libxc_abacus.h and exx_info.h.
  • All five force_stress_lcao TUs carry the same 27 includes. force_stress_lcao_pw.cpp is 73 lines of matrix accumulation but pulls in the full DeePKS / DFT+U / surchem / vdw / TDDFT / operator_lcao header set.
  • The pre-existing duplicate #include "source_io/module_parameter/parameter.h" (already present twice on develop) got replicated into four more TUs.

AGENTS.md rule 3 is "Keep header dependencies minimal". Together with the duplicated whole-class instantiations from my previous review, this is the concrete mechanism by which the split can increase total compile work rather than reduce it.

3. The three new xc_functional.h functions should be private

gradcorr_prepare_rho, gradcorr_xc_kernel and gradcorr_assemble_vxc each have exactly one call site, all inside gradcorr in xc_grad.cpp, but they were added to the public: section. Moving them to private: is free and avoids turning three 24-27 parameter internal helpers into public API.

4. Stale file header in xc_grad.cpp

The header comment still reads:

// it contains 5 subroutines:
// 1. gradcorr ... 2. grad_wfc ... 3. grad_rho ... 4. grad_dot ... 5. noncolin_rho

After the split the file contains only gradcorr; the other four live in xc_grad_utils.cpp and xc_grad_wfc.cpp, neither of which has a header comment. This navigation comment now points readers to the wrong file.

5. Repository governance checker output

python tools/03_code_analysis/agent_governance_check.py \
  --base 5fb17a6ac --head <pr head> --format text

Exit code 0, no ERROR, but two WARNINGs:

  • Test evidence review: Source code changed without test path changes or PR test evidence.
  • Documentation sync review: Source changes have no docs change or explicit no-docs-needed statement.

Both look legitimate here. AGENTS.md rule 6 requires focused tests for core-module refactors, and AGENTS.md also asks contributors to "Report the exact verification performed". The PR description is currently a single line with no verification statement, which matters more than usual given the xc_grad memory-management rewrite. Stating which suites were run (or why tests are not required) would resolve both warnings.

Things that check out

  • Global dependency budget is net -13 (323 GlobalV/GlobalC/PARAM references added, 336 removed), so rule 1's migration-neutral requirement is satisfied.
  • Rules 8 and 9 show exactly symmetric add/remove counts — MPI_Barrier 6/6, MPI_COMM_WORLD 13/13, MPI_Send 5/5, MPI_Recv 5/5, comma-separated declarations 4/4 — so these are pure moves with no new violations introduced.
  • The new[]/delete[] to std::vector conversion in xc_grad matches the AGENTS.md guidance to "Prefer std::vector over raw new/delete for dynamic arrays".

abacus_fixer added 6 commits August 29, 2026 18:07
Commit 385e5d5 replaced the inline radial_in_q[wannier_index].c
expressions with a radial_c variable but left it self-initialized,
so produce_trial_in_pw dereferenced an indeterminate pointer during
Amn/Mmn trial orbital interpolation and crashed (101_PW_W90 segfault).

Initialize radial_c from radial_in_q[wannier_index].c. Verified by
rebuilding abacus_max_para and rerunning the 101_PW_W90 case with
4 MPI processes: etotref, etotperatomref, CompareAMN, CompareMMN and
CompareEIG all pass; agent_governance_check reports no findings.
Same-binary reruns at np=4/OMP=2 showed the stress-sum deviation
swinging between +1.9 and -12.7 kbar run-to-run, matching the
documented non-reproducible MPI reduction order in the EXX/LibRI
sums of this ill-conditioned metallic case. Also record the
observed stress spread in the threshold comment.
The test_stress parts printout in Force_Stress_LCAO::getForceStress
omitted the EXX term, unlike the PW side (Stress_PW::cal_stress).
Add the EXX stress entry so hybrid runs can directly inspect this
contribution, which is a major component in LCAO HSE calculations.
The xc_grad.cpp header comment was updated in the split commit, but the
five remaining xc_grad*.cpp files carried no file-level comment. Add one
to each so the pipeline layout (prepare -> kernel -> assemble) and the
auxiliary utilities stay discoverable without reading xc_functional.h.
The split TUs carried a verbatim copy of the original xc_functional.cpp
include block regardless of actual use. Trim each file to what it needs:
- ATen headers and xc_functional_op.h stay only in xc_grad_wfc.cpp,
  the only TU using ct::Tensor and the operator kernels
- libxc_abacus.h stays only in xc_grad_kernel.cpp, the only TU calling
  XC_Functional_Libxc entry points (XC_GGA_C_LYP in xc_grad.cpp comes
  from xc.h/xc_ids.h already included by xc_functional.h)
- exx_info.h, source_io/module_parameter/parameter.h and unused
  timer/constants/pw_basis_k includes are removed everywhere

Verified with the GNU Make build (non-__LIBXC path) and the CMake xc_
target (__LIBXC path), both compile cleanly.
Makefile.Objects:
- Register the new .cpp files split in this PR in their object groups
- Fix dangling entries: xc_funct_hcth.o -> xc_hcth.o, remove
  onsite_proj_pw.o, memory.o -> memory_recorder.o, dftu.o ->
  dftu_lcao.o and other renamed leftovers
- Update VPATH for renamed directories (source_lcao/module_hcontainer
  and module_gint moved to source_hamilt, add source_io subdirectories)
- Add an explicit symm_rho_charge.o rule target for
  source_estate/module_charge/symm_rho.cpp: two symm_rho.cpp files
  share the name and VPATH resolves to the source_cell one, leaving
  Symmetry_rho undefined (pre-existing on develop)
- Remove the duplicate setup_dftu_pw.o from OBJS_DFTU and drop the
  orphan dmr_complex.o, which no CMakeLists.txt compiles and which
  duplicates a DensityMatrix::cal_DMR specialization

Makefile:
- Set .DEFAULT_GOAL to abacus and pass -f to the sub-make so the
  default target actually builds instead of stopping after the first
  rule (the Intel Makefile CI job has been a 0-second no-op because
  the first rule only generated build_info.h)

generate_build_info.sh:
- Locate build_info.h.in relative to the script directory so the
  header is generated regardless of the caller's cwd

Verified: full GNU Make build with mpicxx links successfully; ABACUS.mpi
--version reports v3.11.0-beta8; tests/01_PW/101_PW_W90 runs to
completion with results matching result.ref; CMake xc_ target still
compiles on both libxc paths.

@Critsium-xy Critsium-xy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK for now. But I notice that wannier_method 1 and 2 have no test cases now. This may need to be added in the future.

@mohanchen
mohanchen merged commit 94576a8 into deepmodeling:develop Aug 31, 2026
17 checks passed
mohanchen pushed a commit to mohanchen/abacus-mc that referenced this pull request Sep 1, 2026
Upstream PR deepmodeling#7873 split onsite_proj.cpp and introduced
onsite_proj_force_stress.cpp, which still called the removed
get_orbital_corr_data() (returning const int*). Our refactor PR had
renamed it to get_orbital_corr_vec() (returning const std::vector<int>&).

Fix by appending .data() at both call sites, matching the new interface.
mohanchen added a commit that referenced this pull request Sep 2, 2026
* Slim down Plus_U_Base interface in dftu_base.h

- Remove dead members with no callers: get_u_target,
  get_orbital_corr_data, deprecated get_pot_uterm_pw(iat),
  element-wise set_occ_mat, and mix_occ_mat (occupation mixing now
  goes through Charge_Mixing::mix_uom + sync_occ_to_uom)
- Merge duplicate occ_mat_initialized accessors: keep the is_*/mark_*
  pair and update dftu_lcao_occ.cpp call sites accordingly
- Replace unitcell.h/charge_mixing.h includes with forward
  declarations in dftu_base.h; add explicit includes to units that
  relied on them transitively (dftu_base.cpp, dftu_cal_occ_pw.cpp,
  dftu_output.cpp) and parallel_comm.h to stodft/hsolver units that
  used BP_WORLD/POOL_WORLD/INT_BGROUP indirectly

Verified: full incremental build of all targets passes with no
errors; repo-wide grep confirms no remaining references to the
removed symbols.

* Fix: include parallel_comm.h for BP_WORLD in hsolver tests

test_hsolver_sdft.cpp and diago_bpcg_test.cpp use BP_WORLD without
including source_base/parallel_comm.h, which broke the MPI build.

* Refactor DFT+U base: unify DFTU_BASE namespace and slim Plus_U_Base

- Rename dftu_output.{h,cpp} -> dftu_base_io.{h,cpp},
  dftu_tools_pw.{h,cpp} -> dftu_base_tools.{h,cpp},
  dftu_cal_occ_pw.cpp -> dftu_base_occ.cpp (git mv)
- Unify all DFTU-related free functions in module_pwdft under a single
  namespace DFTU_BASE (formerly dftu_io, dftu_pw, and pw::iter_init_dftu_pw)
- Extract read_occup_m and local_occup_bcast from Plus_U_Base as
  DFTU_BASE:: free functions declared in dftu_base_io.h; dependencies
  (occ_mat, orbital_corr, occ_mat_ctrl) are now passed explicitly
- Replace element-wise MPI_Bcast in local_occup_bcast with whole-matrix
  Parallel_Common::bcast_double calls (rule: no direct MPI usage)
- dftu_lcao_test links one extra light source (dftu_base_io.cpp) so that
  the heavy PW implementation dftu_base_occ.cpp stays out of the test
  closure
- Move JacobiRotate/CalculateEigenvalues helpers into an anonymous
  namespace and drop an unused local variable during migration

Verification: full incremental build passes in build/ (abacus_basic_para,
100%); in build_max_para_test/ the targets dftu_lcao_test and
dftu_pw_test build and link successfully (100%); grep confirms no
references remain to dftu_io, dftu_pw::, dftu_output, dftu_tools_pw,
dftu_cal_occ_pw, or the removed Plus_U_Base members.

* refactor(dftu): add YukawaScreening class skeleton (no behavior change)

Introduce YukawaScreening to own the Yukawa screening length, Slater
integrals Fk and derived U_Yukawa/J_Yukawa values. The implementation is
moved verbatim from DFTU_LCAO free functions; no call sites are changed
yet, so dftu_yukawa.* stays in place and behavior is unchanged.

Verification: make -j 30 dftu (build_max_para_test) passed.

* refactor(dftu): hold YukawaScreening in Plus_U_Base

Move yukawa_screening.* from module_dftu to module_pwdft so the base
layer can own it without depending on LCAO. Plus_U_Base now constructs a
YukawaScreening when use_yukawa_ is set and exposes it via yukawa(); the
legacy lambda/Fk/U_Yukawa/J_Yukawa members are kept in place until call
sites migrate. The cal_slater_Fk orb dependency stays guarded by __LCAO.

Verification: make -j 30 module_pwdft (build_max_para_test) passed.

* refactor(dftu): switch call sites to YukawaScreening, drop dftu_yukawa.*

Replace the DFTU_LCAO free Yukawa functions with the YukawaScreening
member owned by Plus_U_Base. setup_dftu_lcao now drives
yukawa().cal_slater_UJ and writes U-J back to u_current; the pots,
energy and IO paths read U/J through yukawa(). The yukawa_lambda config
is threaded through init_base into YukawaScreening::init, and
dftu_yukawa.h/.cpp are removed.

Verification: make -j 30 (build_max_para_test) passed;
ctest -R dftu: 4/4 passed (dftu_pw_test, dftu_core_test,
dftu_operator_test, dftu_lcao_test).

* refactor(dftu): drop legacy Yukawa members from Plus_U_Base

All Yukawa state (lambda, Slater Fk integrals, derived U/J) now lives
exclusively in YukawaScreening, owned by Plus_U_Base via yukawa_.
use_yukawa() is derived from the pointer instead of a separate flag,
and the redundant yukawa_lambda member/accessor on Plus_U is removed.

Verified: make -j 30 in build_max_para_test passes;
OMP_NUM_THREADS=1 ctest -R dftu passes (4/4).

* refactor(dftu): add OccupationMatrix skeleton (unused for now)

New class owning the nested occ[iat][l][n][spin] matrices, their saved
copy for mixing, and the iat->(l,n,m,ipol)->iwt lookup table. Provides
element/matrix access, flat (de)serialization and bulk zero/copy/flat
operations covering everything the legacy Plus_U_Base code paths need.
Not yet referenced by existing code; call sites switch in later steps.

Verified: make -j 30 in build_max_para_test passes.

* refactor(dftu): Plus_U_Base holds an OccupationMatrix instance

occmat_ is allocated alongside the legacy occ_mat/occ_mat_save/
iatlnmipol2iwt arrays in init_base, and exposed via occmat(). Existing
read/write paths are unchanged; writers switch to occmat_ next.

Verified: make -j 30 in build_max_para_test passes;
OMP_NUM_THREADS=1 ctest -R dftu passes (4/4).

* Refactor: route DFT+U occupation-matrix writes through OccupationMatrix

Switch all internal writers and public read accessors of Plus_U_Base from
the legacy occ_mat/occ_mat_save/iatlnmipol2iwt members to the new
occmat_ (OccupationMatrix) object:

- init_base: read_occup_m/local_occup_bcast now fill occmat_.data()
- copy_occ_mat -> occmat_.copy_to_save + write_save_to_flat
- zero_occ_mat  -> occmat_.zero
- set_occ_mat   -> occmat_.read_from_flat
- get/set_occ_mat_flat -> occmat_.get_flat/set_flat
- dftu_base_occ.cpp: reduce_occ_mat, sync_occ_to_uom,
  compute_eff_pot_and_energy, accumulate_occ_one_k use occmat_.mat()
- header accessors get_occ_mat/get_occ_mat_save/get_occ_mat_data/
  get_occ_mat_save_data/get_iatlnmipol2iwt forward to occmat_
- dftu_lcao_test builds and reads the matrices through occmat()

The legacy members are still allocated but now dead; they are removed in
the follow-up commit. Behavior is unchanged: make -j 30 and
OMP_NUM_THREADS=1 ctest -R dftu (4/4) pass.

* Refactor: remove legacy DFT+U occupation-matrix members

All readers and writers now go through occmat_ (OccupationMatrix), so the
legacy Plus_U_Base members occ_mat, occ_mat_save and iatlnmipol2iwt are
deleted together with their allocation block in init_base (the
pot_uterm_pw_index / num_locale bookkeeping is kept).

The IO free functions read_occup_m / local_occup_bcast now take an
OccupationMatrix& instead of the raw nested vector, and write into it
through the set()/mat() interface. write_occup_m / output already used
the public get_occ_mat() accessors and are unchanged.

Behavior is unchanged: make -j 30 and OMP_NUM_THREADS=1 ctest -R dftu
(4/4) pass.

* docs: record nested-vector member extraction pattern in AGENTS.md

Lesson from the DFT+U OccupationMatrix refactor (B2-B4): migrate a
base-class nested-vector member in three steps so no commit mixes
old-storage writes with new-storage reads.

* refactor: move OccupationMatrix from module_pwdft to source_estate

OccupationMatrix depends only on UnitCell and ModuleBase::matrix, not on
any PW- or LCAO-specific layer. It is an electronic-state data container,
so source_estate is a more natural home than source_pw/module_pwdft.

Verified with make -j 30 in build_max_para_test and
OMP_NUM_THREADS=1 ctest -R dftu (4/4 passed).

* refactor: drop Plus_U_Base pure-forwarding occupation accessors

Remove 8 accessors on Plus_U_Base that only forwarded to the
OccupationMatrix member; call sites now use dftu.occmat() directly:
  get_iatlnmipol2iwt, get_occ_mat, get_occ_mat_save, get_occ_mat_data,
  get_occ_mat_save_data, get_occ_mat_flat, set_occ_mat_flat
(get_occ_mat_flat/set_occ_mat_flat definitions in dftu_base.cpp removed too).

Kept occmat(), get_occ_mat_ctrl(), and the copy_occ_mat/zero_occ_mat/
set_occ_mat/sync_occ_to_uom wrappers, which still touch the uom_array /
uom_save / pot_uterm_pw_index base-class members.

Verified with make -j 30 in build_max_para_test and
OMP_NUM_THREADS=1 ctest -R dftu (4/4 passed).

* fix: guard YukawaScreening::cal_slater_Fk body with __LCAO

cal_slater_Fk dereferences const LCAO_Orbitals*, but the complete type
(orb_read.h) is only included under #ifdef __LCAO while the function body
was not guarded. PW-only CI builds (without __LCAO) failed with
"invalid use of incomplete type 'const class LCAO_Orbitals'".

Guard the body with #ifdef __LCAO and WARNING_QUIT in the non-LCAO stub,
matching the existing codebase pattern (vnl_pw_alpha.cpp, esolver_factory).

Verified: make -j 30 (LCAO build) passed;
c++ -fsyntax-only -std=gnu++11 (no __LCAO) passed;
OMP_NUM_THREADS=1 ctest -R dftu 4/4 passed.

* refactor(dftu): slim dftu_lcao.h and turn pot_uterm_HR into free functions

Header slimming (forward declarations):
- dftu_lcao.h: drop 7 transitive includes (klist/unitcell/parallel_orbitals/
  orb_read/hamilt/hcontainer/density_matrix), replace with forward
  declarations for UnitCell, Parallel_Orbitals, LCAO_Orbitals,
  hamilt::HContainer<T> and elecstate::DensityMatrix<TK,TR>.
- Add direct includes where the removed transitives were actually used:
  dftu_lcao.cpp, dftu_lcao_pots.cpp, dftu_lcao_energy.cpp,
  force_stress_lcao.h, write_hs_r.h, spar_u.cpp.

Free-function refactor:
- Move Plus_U::cal_eff_pot_mat_R_double / cal_eff_pot_mat_R_complex_double
  out of the class into namespace DFTU_LCAO, renamed to pot_uterm_HR_real /
  pot_uterm_HR_complex to match the existing pot_uterm_real/complex family.
- spar_u.cpp now calls DFTU_LCAO::pot_uterm_HR_real/complex(dftu, ...).

Verified: cmake --build build -j4 (exit 0). This build has no test targets.

* refactor(dftu): sink occupation mixing to source_estate and drop forwarding methods

- Add free function elecstate::mix_occ_with_save() in occ_matrix.{h,cpp}:
  occ = beta*occ + (1-beta)*occ_save over each atom's correlated orbital,
  nspin-aware (nspin=4 single Pauli block, nspin=1/2 both spin channels).
- LCAO cal_occ_mat_k/gamma: replace the two duplicated hand-written mixing
  loops with a single mix_occ_with_save() call (~60 lines removed).
- Plus_U_Base: delete the pure-forwarding methods copy_occ_mat /
  zero_occ_mat / set_occ_mat and inline their call sites
  (init_base in dftu_base.cpp, cal_occ_pw in dftu_base_occ.cpp) to call
  occmat_.copy_to_save / write_save_to_flat / zero / read_from_flat directly.

Verified: cmake --build build -j4 (exit 0). This build has no test targets.

* refactor(dftu): turn Plus_U_Base PW occ path into DFTU_BASE free functions

Move the remaining PW occupation-matrix members out of Plus_U_Base into
namespace DFTU_BASE (declared in dftu_base_tools.h, defined in
dftu_base_occ.cpp):
- reduce_occ_mat(cell, nspin, kpar, orbital_corr, occmat): k-pool reduce
  still goes through Parallel_Reduce::reduce_double_allpool (unchanged MPI
  wrapper).
- compute_eff_pot_and_energy -> compute_pot_uterm_and_energy(...), renamed to
  match the pot_uterm_* family; takes u_current/orbital_corr/
  pot_uterm_pw_index/occmat/pot_uterm_pw and writes energy_u by reference.
- accumulate_occ_one_k<Device>: templated free function, explicit
  instantiations for DEVICE_CPU (and DEVICE_GPU) moved to the .cpp.
- sync_occ_to_uom removed; cal_occ_pw now calls occmat_.write_to_flat
  directly.

Plus_U_Base::cal_occ_pw remains as the thin orchestration wrapper.

Verified: cmake --build build -j4 (exit 0). This build has no test targets.

* refactor(dftu): collapse cal_occ_mat_k/gamma signatures onto Plus_U&

cal_occ_mat_k: 13 params -> 9 (add Plus_U& dftu, drop nspin/npol/nlocal/
ks_solver/iatlnmipol2iwt/orbital_corr/occ_mat/occ_mat_save/occ_mat_initialized).
cal_occ_mat_gamma: 13 params -> 7 (add Plus_U& dftu).

Both now read all occupation-matrix state from dftu.occmat()
(mat/mat_save/data/data_save/iatlnmipol2iwt/nspin/npol) and the Plus_U_Base
accessors (get_orbital_corr_vec / is_occ_mat_initialized /
mark_occ_mat_initialized), and use occmat().copy_to_save()/zero() instead of
the duplicated hand-written copy/zero loops. The intermediate cal_occ_mat
template forwarders shrink to a single direct call.

MPI_Allreduce(MPI_COMM_WORLD) inside the accumulation is intentionally left
unchanged: it has no exact Parallel_Reduce counterpart and was kept per the
"only replace what maps cleanly" rule.

Verified: cmake --build build -j4 (exit 0). This build has no test targets.

* refactor(dftu): split DFT+U force/stress into r-space NAO files and rename for clarity

Rename and reorganize DFT+U force/stress implementation:

New files (r-space, NAO basis):
- dftu_nao_fs_r.h/cpp    : unified force+stress entry with full formula docs
- dftu_nao_for_r.h/cpp   : single-pair force core (cal_for_IJR_nao_r)
- dftu_nao_str_r.h/cpp   : single-pair stress core (cal_str_IJR_nao_r)

Renamed files:
- dftu_lcao_op.h/cpp          -> dftu_nao_op.h/cpp
- dftu_lcao_op_legacy.cpp     -> dftu_nao_op_legacy.cpp
- dftu_force.h/cpp (legacy k) -> dftu_nao_fs_k.h/cpp
- dftu_folding.h/cpp          -> dftu_nao_folding.h/cpp

Deleted:
- dftu_fs.cpp (split into dftu_nao_fs_r.cpp + dftu_nao_for_r.cpp + dftu_nao_str_r.cpp)

Updated:
- dftu_nao_op.h: expose getters and cal_pot_onsite/transfer_pot_onsite for free functions
- force_stress_lcao.cpp, hamilt_lcao.cpp, dftu_lcao_occ.cpp: update includes
- test/CMakeLists.txt, test/dftu_lcao_test.cpp: update source list and include

Naming convention:
- nao : NAO/LCAO basis set
- _r  : real-space implementation (uses DMR, k-point independent)
- _k  : k-space legacy implementation (deprecated, broken)
- for : force
- str : stress
- fs  : force+stress combined

Co-Authored-By: TRAE <noreply@trae.ai>

* increase the thr of stress from 11 to 12 in example 15_KP_HSE_SOC_symm

* refactor(dftu): rename all dftu_lcao* files to dftu_nao* for NAO basis clarity

Rename all LCAO-specific DFT+U files in module_dftu to use 'nao' instead
of 'lcao', making the basis-set dependency explicit in the filename:

- dftu_lcao.h/cpp          -> dftu_nao.h/cpp
- dftu_lcao_occ.h/cpp      -> dftu_nao_occ.h/cpp
- dftu_lcao_pots.h/cpp     -> dftu_nao_pots.h/cpp
- dftu_lcao_energy.h/cpp   -> dftu_nao_energy.h/cpp
- dftu_lcao_op_legacy.h    -> dftu_nao_op_legacy.h

Update all #include references across 29 files in:
  source_lcao, source_esolver, source_estate, source_io, module_dftu

Update CMakeLists.txt source lists accordingly.

Note: setup_dftu_lcao.cpp/h keep their names (they are not in module_dftu
and represent the DFT+U setup workflow, not a basis-specific implementation).

Co-Authored-By: TRAE <noreply@trae.ai>

* fix: prevent out-of-bounds write in write_save_to_flat for nspin=1

For nspin==1, uom_save is allocated as a single block (pot_index is
doubled only when nspin==2). However, write_save_to_flat previously
wrote both spin channels unconditionally, causing part of the first
block to be overwritten and indices past the vector end to be accessed
(undefined behavior via operator[]).

Fix by guarding the channel-[1] write with `if (nspin_ == 2)`,
consistent with write_to_flat() and read_from_flat() which already
handle the two channels correctly.

* fix: adapt onsite_proj_force_stress.cpp to renamed Plus_U_Base interface

Upstream PR #7873 split onsite_proj.cpp and introduced
onsite_proj_force_stress.cpp, which still called the removed
get_orbital_corr_data() (returning const int*). Our refactor PR had
renamed it to get_orbital_corr_vec() (returning const std::vector<int>&).

Fix by appending .data() at both call sites, matching the new interface.

* fix(Makefile): update stale DFTU object names after module_dftu rename

The DFT+U LCAO files in module_dftu/ were renamed from dftu_lcao_*
to dftu_nao_* (step 6, commit 4fd1529), but source/Makefile.Objects
still referenced the old names. Additionally, dftu_fs.cpp was split
into four focused files (dftu_nao_fs_k/for_r/fs_r/str_r), so the old
single dftu_fs.o entry is no longer valid.

Fix both OBJS_LCAO and OBJS_DFTU sections to use the current source
filenames so the legacy Makefile build (used by the Intel CI) works.

* fix: eliminate variadic macro warnings in REQUIRES_OK

Two related warnings were triggered:
- -Wvariadic-macro-arguments-omitted: memory.h called REQUIRES_OK
  with only one argument (no variadic arg)
- -Wgnu-zero-variadic-macro-arguments: macros.h used GNU extension
  '##__VA_ARGS__' to swallow the comma when __VA_ARGS__ was empty

Fix:
1. Add a message argument to the single-arg REQUIRES_OK call site
2. Drop the '##' GNU extension since all call sites now pass at
   least one variadic argument

* fix: remove trailing semicolons in xc_kernel.h macros and destructor

The CREF and CREF3 macro definitions each had a trailing semicolon,
and the call sites also added one, resulting in double semicolons
(;;) inside the class that triggered -Wextra-semi warnings.

Remove the semicolons from the macro definitions and from the
destructor definition (~KernelXC() {};) to eliminate the warnings.

Verified: cmake --build . completes with no warnings from xc_kernel.h.

* fix: suppress -Wundefined-var-template warning for OperatorEXXPW::fock_div

Replace redundant explicit instantiation definitions (template class)
with explicit instantiation declarations (extern template class) in
op_pw_exx_pot.cpp. The static data member fock_div is defined and
instantiated in op_pw_exx.cpp; the duplicates in op_pw_exx_pot.cpp
could not instantiate it (out-of-class definition not visible) and
triggered -Wundefined-var-template when get_exx_potential accessed it.

* fix: add override to ElecStatePW::psiToRho and cal_tau

ElecStatePW<T, Device> is a template derived class. Its psiToRho/cal_tau
signatures only match the base-class virtual for some (T, Device) combos,
which prevented simply adding 'override' to the template declaration.

Fix: enumerate all (T, Device) combos (CPU and GPU for complex<float>,
complex<double>, and double) as empty virtuals in the base class ElecState,
then mark the two derived-class template members as 'override'.

This resolves the -Winconsistent-missing-override warning from icpx/clang
without changing any runtime behavior.

* fix(Makefile): drop stale dftu_yukawa.o from OBJS_DFTU

Commit 8310580 removed dftu_yukawa.cpp/.h and dropped it from the module_dftu CMakeLists, but source/Makefile.Objects still listed dftu_yukawa.o in OBJS_DFTU. The subsequent rename fix (5f3af73) updated the other DFTU object names but left this orphan entry behind, so the Intel CI Makefile build failed with 'No rule to make target build/obj/dftu_yukawa.o, needed by build/bin/ABACUS.mpi'.

Verified: make -n -f ../Makefile abacus from source/build now exits 0 with no missing-target error; the link line lists the 11 valid DFTU objects and no dftu_yukawa.o. dftu_nao_op.o is already registered in OBJS_HAMILT_LCAO, so no replacement is needed.

* fix(cal_plpr): resolve -Wreturn-type warnings under NDEBUG

In release builds with -DNDEBUG, assert(false) is compiled out, leaving
cal_LxijR and cal_LyijR with control paths that fall off the end without
returning a value. Convert the trailing `if (jm < -1)` to `else` and move
assert(jm < -1) inside as a defensive check, so the compiler can statically
prove all paths return. Also add explicit #include <cassert>.

* fix(ions_move_lbfgs): replace VLA with std::vector to fix -Wvla-cxx-extension

Replace the variable-length array 'double a[3*size]' in update_pos with a
std::vector<double>. The VLA relied on a runtime member 'size' and was a
non-standard Clang extension. The vector's .data() is passed to
unitcell::update_pos_tau, whose 'pos' parameter is 'const double*', so
the conversion is semantically equivalent.

* fix: resolve -Wdtor-name warning in EKinetic destructor definition

Use the fully-qualified template name after '::~' to satisfy Intel
compiler's -Wdtor-name, matching the pattern already used in
Mix_DMk_2D<Tdata>::~Mix_DMk_2D<Tdata>(). C++11 compatible.

* Fix -Wdtor-name warning in OnsiteProjector destructor

Move the out-of-line destructor definition and its explicit
instantiations into a 'namespace projectors {}' block so the name
after '~' is looked up in the same scope as the class name, satisfying
ISO C++ requirements.

* fix: replace C99 _Complex with std::complex<double> in cblacs.h

C99 `_Complex` is not valid in C++, causing -Wc99-extensions warnings
when cblacs.h is included from C++ translation units (wrapped in
extern "C" blocks). Replace with `std::complex<double>`, which has
identical memory layout and is compatible with the BLACS C library.

* fix(dftu): replace VLAs with std::vector in dftu_nao_fs_r

Replace two variable-length arrays (dmR_tmp and tmp) sized by
dftu_op->get_nspin() with std::vector, eliminating the
-Wvla-cxx-extension warnings. Both arrays are now explicitly
zero-initialized with nullptr. Pass tmp.data() to cal_for_IJR_nao_r
and cal_str_IJR_nao_r to match their const BaseMatrix<double>**
parameter signature.

* fix: wrap member function definitions in namespace hamilt for -Wdtor-name

Move Nonlocal and Overlap member function definitions inside `namespace hamilt`
blocks and drop redundant `hamilt::` qualifiers. This resolves the ISO C++
warning:
  warning: ISO C++ requires the name after '::~' to be found in the
           same scope as the name before '::~' [-Wdtor-name]

Affected files:
  - source/source_lcao/module_operator_lcao/nonlocal.cpp
  - source/source_lcao/module_operator_lcao/overlap.cpp

* fix: add missing variadic macro argument in Tensor::sync

REQUIRES_OK(expr, ...) is a variadic macro defined in macros.h.
C++14 requires at least one argument be passed for the ... parameter;
omitting it is a C++20 extension that triggers -Wvariadic-macro-arguments-omitted
under icpx -pedantic.

Tensor::sync was the only call site in the whole codebase that omitted the
second argument. Add a descriptive error message to keep the macro usage
consistent with every other REQUIRES_OK call.

* build(make): link occ_matrix.o to fix undefined reference to OccupationMatrix::get_flat

occ_matrix.cpp was missing from Makefile.Objects, so the Intel Makefile
build did not compile/link occ_matrix.o, causing an undefined reference
to OccupationMatrix::get_flat in dftu_nao_op.cpp. Add it to OBJS_ELECSTAT
to mirror source_estate/CMakeLists.txt, which already lists occ_matrix.cpp
in the elecstate object library.

* fix(Makefile): resolve multiple definitions and missing YukawaScreening link

- Remove 4 duplicate objects (dftu_nao_fs_k/for_r/fs_r/str_r.o) from
  OBJS_HAMILT_LCAO; they are already listed in OBJS_DFTU and both
  lists merge into OBJS_ABACUS, causing "multiple definition" linker errors.
- Add missing yukawa_screening.o to OBJS_SRCPW so YukawaScreening::*
  symbols resolve; it is already present in CMakeLists.txt module_pwdft.

Verified with `make -n` (dry-run): dftu_nao_fs_k.o appears exactly once
in the final link line; yukawa_screening.o is compiled from
source_pw/module_pwdft/yukawa_screening.cpp.

* fix(dftu): clear stale YukawaScreening when init_base reruns with yukawa_potential=false

Before the refactor, use_yukawa_ was assigned on every init_base() call,
so the state always matched the latest argument. Now the state is
inferred from yukawa_, but the pointer was only updated on the true
branch; a true -> false re-initialization left a stale object alive.
PW's before_scf() -> setup_pot() may call init_base() repeatedly on the
same Plus_U object, so clear the pointer in the disabled branch to
preserve the old semantics. Add unit tests for both switch directions
(reverse-verified: they fail with the stale-object behavior).

* fix(dfpt): use mark_occ_mat_initialized in pw_data test

The test called a nonexistent Plus_U_Base::set_occ_mat_initialized(bool);
the actual API is mark_occ_mat_initialized(). Verified locally:
MODULE_DFPT_pw_data_test 6/6 passed.

---------

Co-authored-by: abacus_fixer <mohanchen@pku.eud.cn>
Co-authored-by: TRAE <noreply@trae.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants