Skip to content

Move Structure_Factor, Soc and the cal_sk_op kernel to source_cell; drop four unused upward includes - #8234

Merged
mohanchen merged 4 commits into
deepmodeling:developfrom
Critsium-xy:refactor/structure-factor-to-cell
Oct 30, 2026
Merged

mohanchen merged 4 commits into
deepmodeling:developfrom
Critsium-xy:refactor/structure-factor-to-cell

Conversation

@Critsium-xy

@Critsium-xy Critsium-xy commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Reminder

  • I have read AGENTS.md and docs/developers_guide/agent_governance.md.
  • I have linked an issue or explained why this PR does not need one.
  • I have added adequate unit tests and/or case tests, or explained why not.
  • I have listed the exact verification commands run and their results.
  • I have described user-visible behavior changes, including INPUT parameter changes.
  • I have explained core-module impact for ESolver, HSolver, ElecState, Hamilt, Operator, Psi, or other source/ changes.
  • I have requested any needed governance exception below.

Linked Issue

Fixes #8233. Part of #7601 and #7599. The second commit also removes four unused upward includes found in the same layering review (see "What's changed?", item 6).

Unit Tests and/or Case Tests for my changes

  • No new tests: the first commit is a file move, the second only deletes include lines. The existing MODULE_PW_pwdft_soc, MODULE_PW_structure_factor_test, MODULE_PW_Hamilt_Kernels_UTs (cal_sk_op), MODULE_HAMILT_surchem_cal_pseudo and MODULE_PSI_init_test cover the moved code. They stay where they are and now compile the files from source_cell.
  • Commands run:
    • python3 tools/03_code_analysis/agent_governance_check.py --staged --format text on each commit: warnings only, listed under Governance Notes.
    • A static check against the parent of each commit: for every file that reaches one of the changed headers, no class, alias or macro it uses stops being declared somewhere in its include closure.
    • On a Linux CPU box (GCC 12.2, CMake 3.31, Open MPI 4.1, ninja):
      • cmake -B build -G Ninja -DBUILD_TESTING=ON -DENABLE_LCAO=ON -DENABLE_MPI=ON -DENABLE_OPENMP=ON then cmake --build build -j15: success at both 31e2afb35 and e85cfd59d. abacus_basic_para reports v3.11.0-beta11.
      • cmake -B build_pw -G Ninja -DBUILD_TESTING=OFF -DENABLE_LCAO=OFF -DENABLE_MPI=ON -DENABLE_OPENMP=ON then build: success at both commits (abacus_pw_para).
      • At 31e2afb35: ctest -j1 -R "MODULE_PW_pwdft_soc|MODULE_PW_structure_factor_test|MODULE_PW_Hamilt_Kernels_UTs|MODULE_HAMILT_surchem_cal_pseudo|MODULE_PSI_init_test|MODULE_CELL_NEIGHBOR|MODULE_DFPT|MODULE_ESTATE_elecstate_pw$|MODULE_LCAO_tddft_snap_psibeta_half_test": 26/26 passed. These are the tests that compile the moved files, plus the ones that link cell (MODULE_CELL_NEIGHBOR_*) or module_pwdft (MODULE_DFPT_*).
      • At e85cfd59d: ctest -j1 -R "MODULE_RELAX|MODULE_ESTATE|MODULE_DM|MODULE_LCAO_tddft": 49/49 passed. These cover the three files of the second commit.
  • Result summary: both builds succeed at both commits, and all directly affected unit tests pass (26 + 49).
    • Third commit 92289fe11 (forwarding headers removed): no local build or test. I checked with git grep that no file still references module_pwdft/stru_fac.h or module_pwdft/soc.h. Build and tests are left to CI.
  • Checks not run, with reason:
    • CUDA and ROCm builds: no GPU toolchain on my build machine; left to CI. The kernel files are moved unchanged apart from their first #include line.
    • MODULE_LCAO_deepks_*, which also compiles soc.cpp: needs ENABLE_MLALGO, which the box lacks; left to CI.
    • Integration suites and the full unit suite: left to CI. No code is changed apart from include lines and build lists.

What's changed?

Structure_Factor holds S(G) = Σ exp(−iG·τ) and the per-atom phase factors exp(−iG·τ): the Fourier representation of the atomic positions. Soc holds the spin–angular coupling coefficients of the pseudopotentials. Neither is plane-wave method code, and lower layers use both, so they move from source_pw/module_pwdft to source_cell. The potential form factors (vl_pw, vnl_pw, vsep_pw) stay in source_pw.

  1. Moved files (contents unchanged apart from include lines):

    • stru_fac.h, stru_fac.cpp, stru_fac_k.cpp, soc.h, soc.cpp → source_cell/
    • the cal_sk_op kernel kernels/wf_op.{h,cpp}, kernels/cuda/wf_op.cu, kernels/rocm/wf_op.hip.cu → source_cell/kernels/. It computes the phase factors for Structure_Factor::get_sk, and is its only user.
  2. Include fixes in the moved files.

    • wf_op.h no longer includes source_psi/psi.h, which it did not use. Without this, source_cell would include source_psi.
    • stru_fac_k.cpp now includes source_base/module_device/memory_op.h directly. It uses the memory ops, and used to get them through psi.h.
  3. No forwarding headers (third commit, following review). source_pw/module_pwdft/stru_fac.h and soc.h are deleted. The 21 remaining includers above source_cell now include source_cell/stru_fac.h or source_cell/soc.h directly:

    • source_esolver: esolver_fp.h
    • source_io: module_bessel/numerical_basis.h, module_ctrl/ctrl_scf_lcao.h, module_energy/write_eband_terms.cpp
    • source_lcao: force_stress_lcao.h, lcao_set.h, setup_nonlocal.cpp (soc.h)
    • source_pw: module_dfpt/dfpt_pert.cpp, dfpt_pw.cpp, test/dfpt_test_mocks.cpp, test_serial/dfpt_{pert,phon,q0}_serial_test.cpp, module_proj/onsite_proj.h, module_pwdft/{force_pw,setup_pot,stress_func}.h, module_pwdft/vnl_pw.h (both headers), module_stodft/sto_stress_pw.cpp

    wf_op.h never had a forwarding header, because its only includers moved with it or are updated here.

  4. Lower-layer includers now use the new paths:

    • source_psi: psi_base.cpp, psi_init_atomic.cpp (both headers), psi_init_nao.cpp, psi_prepare.cpp, setup_psi_pw.cpp, setup_psi_pw.h
    • source_estate: module_charge/chg_extra.h, module_pot/potential_new.h, setup_estate_pw.h
    • source_hamilt: module_surchem/cal_pseudo.cpp
    • tests: psi_init_unit_test.cpp, test_elecstate_pw.cpp, module_surchem/test/setcell.h, test_soc.cpp, test_stru_fac.cpp, wf_op_test.cpp (names as of the latest develop merge, which renamed some test files)

    After this, no file in source_psi, source_estate or source_hamilt includes stru_fac.h or soc.h from source_pw. source_hamilt no longer includes source_pw at all.

  5. Build.

    • soc.cpp joins the cell object library. It only needs source_base.
    • stru_fac.cpp and stru_fac_k.cpp form a new structure_factor object library in source_cell/CMakeLists.txt, linked into the executable next to cell. They are not added to cell because they need planewave, which the cell-only unit tests (MODULE_CELL_NEIGHBOR_*) link cell without.
    • The kernel entries in source/CMakeLists.txt (CPU, CUDA, ROCm) point to source_cell/kernels/.
    • Six unit-test source entries, in five CMakeLists.txt files, point to the new paths. The develop merge also updates source_pw/module_rt/unittests/CMakeLists.txt (new upstream in Feature: Enable noncollinear magnetism and SOC in PW RT-TDDFT with the ETRS propagator and adaptive time stepping #8223), which listed module_pwdft/{stru_fac,stru_fac_k,soc}.cpp.
    • source/Makefile.Objects: ./source_cell/kernels is added to VPATH. The object names are unchanged.
    • python/pyabacus does not list these files.
  6. Unused upward includes (second commit). These lines include a higher layer but use nothing from it:

    • source_estate/module_dm/edm_tddft.cpp: source_lcao/module_rt/gather_mat.h and propagator.h. Only edm_tddft_lapack.cpp uses them, and it keeps its own includes.
    • source_estate/elecstate_pw_sdft.cpp: source_hamilt/module_xc/xc_functional.h; no XC symbol is used.
    • source_relax/ions_move_lbfgs.h: source_esolver/esolver_ks.h. The class only stores an ESolver* (the code that assigned it is commented out), and esolver.h, still included, declares it. This keeps the whole KS solver header out of every includer of ions_move_lbfgs.h.

    After this, edm_tddft.cpp no longer includes source_lcao.

Governance Notes

  • INPUT/docs changes: none. No INPUT parameter, output or documented interface changes, so no documentation update is needed.
  • Core module impact: source_cell gains Structure_Factor, Soc and one kernel. source_psi, source_estate, source_hamilt, source_relax, source_esolver, source_io, source_lcao and source_pw only change include lines.
  • Exceptions requested:
    • Global dependency budget: net 0. Structure_Factor::Structure_Factor() reads PARAM.inp.basis_type and PARAM.inp.device; the lines move with the file unchanged. Passing these in explicitly is a separate cleanup.
    • Header include warnings: every flagged line replaces source_pw/module_pwdft/{stru_fac,soc}.h with the same header at its new path.
  • Follow-ups, not in this PR:
    • The tests of the moved classes still live in source_pw/module_pwdft/unittests and kernels/test. Moving them into source_cell is left out to keep this diff to the move itself.

🤖 Generated with Claude Code

Critsium-xy and others added 2 commits October 29, 2026 16:40
Structure_Factor holds S(G) and the per-atom phase factors exp(-iG.tau),
the Fourier representation of the atomic positions. Soc holds the
spin-angular coupling coefficients of the pseudopotentials. Neither is
plane-wave method code, but both lived in source_pw/module_pwdft, so
source_psi, source_estate and source_hamilt included source_pw for them.

- stru_fac.{h,cpp}, stru_fac_k.cpp, soc.{h,cpp} move to source_cell/.
  soc.cpp joins the cell library. stru_fac*.cpp form a separate
  structure_factor object library, because they need the plane-wave
  basis and the cell-only unit tests do not link planewave.
- The cal_sk_op kernel (wf_op.{h,cpp}, cuda/wf_op.cu, rocm/wf_op.hip.cu)
  moves to source_cell/kernels/. Its unused include of source_psi/psi.h
  is dropped; stru_fac_k.cpp now includes memory_op.h, which it used
  through psi.h.
- The old headers in source_pw/module_pwdft forward to the new ones, so
  higher-layer includers are unchanged. The includers in source_psi,
  source_estate and source_hamilt, and the tests that compile these
  files, point at the new paths.

No code inside the moved files changes apart from include lines.

Refs deepmodeling#8233

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- edm_tddft.cpp includes module_rt/gather_mat.h and propagator.h but
  uses neither; only edm_tddft_lapack.cpp needs them.
- elecstate_pw_sdft.cpp includes xc_functional.h but uses no XC symbol.
- ions_move_lbfgs.h includes esolver_ks.h, but the class only stores an
  ESolver*, which esolver.h already declares.

Refs deepmodeling#8233

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Critsium-xy Critsium-xy changed the title Move Structure_Factor, Soc and the cal_sk_op kernel from source_pw to source_cell Move Structure_Factor, Soc and the cal_sk_op kernel to source_cell; drop four unused upward includes Oct 29, 2026
@mohanchen mohanchen added Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0 labels Oct 29, 2026
Comment thread source/source_pw/module_pwdft/soc.h Resolved
Critsium-xy and others added 2 commits October 29, 2026 22:05
Point the remaining includers in source_esolver, source_io, source_lcao
and source_pw at source_cell/stru_fac.h and source_cell/soc.h directly,
and delete the two forwarding headers left in source_pw/module_pwdft.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…re-factor-to-cell

# Conflicts:
#	source/source_pw/module_pwdft/unittests/CMakeLists.txt

@mohanchen mohanchen 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.

LGTM

@mohanchen
mohanchen merged commit 64463eb into deepmodeling:develop Oct 30, 2026
19 checks passed
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.

[Dependency] Structure_Factor and Soc live in source_pw, but source_psi, source_estate and source_hamilt depend on them

2 participants