Skip to content

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

Closed

Description

@Critsium-xy

Describe the Code Quality Issue

Structure_Factor and Soc live in source_pw/module_pwdft, but they are not plane-wave method code, and lower layers depend on them:

  • Structure_Factor holds the structure factor S(G) = Σ exp(−iG·τ) and the phase factors exp(−iG·τ) per atom. It is the Fourier representation of the atomic positions, i.e. geometry. ESolver_FP owns it (source_esolver/esolver_fp.h:78) for every basis type, and LCAO code uses it too.
  • Soc holds the spin–angular coupling coefficients of the pseudopotentials, i.e. atomic data. It depends only on source_base.
  • The cal_sk_op kernel (source_pw/module_pwdft/kernels/wf_op.h, with CPU, CUDA and ROCm implementations) computes the same phase factors from atom positions for Structure_Factor::get_sk.

The potential form factors (vl_pw, vnl_pw, vsep_pw) are basis-specific and do belong in source_pw. Together with S(G) they give V(G) = Σ S(G)·v(G).

Because of this placement, lower layers include source_pw:

  • source_psi → source_pw (the wavefunction initializers need S(G) and Soc):
    • psi_base.cpp:6
    • psi_init_atomic.cpp:13 (soc.h), :14
    • psi_init_nao.cpp:5
    • psi_prepare.cpp:23
    • setup_psi_pw.cpp:7
    • setup_psi_pw.h:11
  • source_estate → source_pw: module_charge/chg_extra.h:9, module_pot/potential_new.h:7, setup_estate_pw.h:6
  • source_hamilt → source_pw: module_surchem/cal_pseudo.cpp:3

Also, kernels/wf_op.h:5 includes source_psi/psi.h but uses nothing from it, only the base_device tags.

Suggested fix

  1. Move stru_fac.h, stru_fac.cpp, stru_fac_k.cpp, soc.h and soc.cpp to source_cell/. Structure_Factor needs only source_base, source_basis/module_pw and source_cell, and source_cell already sits above source_basis (source_basis includes nothing from source_cell).
  2. Move the cal_sk_op kernel with them, to source_cell/kernels/, and drop the unused psi.h include.
  3. Leave forwarding headers at the old paths, so the higher-layer includers (source_pw, source_lcao, source_esolver, source_io) need no change in the same PR. Only the lower-layer files listed above change their include line.
  4. Update the build lists: the source_pw/module_pwdft, source_cell and kernel lists in CMake, the unit-test lists that compile these files, and source/Makefile.Objects.

No behavior change should result.

Acceptance

  • source_psi, source_estate and source_hamilt no longer include stru_fac.h or soc.h from source_pw.
  • The CPU, CUDA and ROCm builds compile, with and without LCAO.
  • Unit tests pass, plus the 01_PW, 02_NAO_Gamma and 03_NAO_multik integration suites.

Additional Context

Task list for Issue attackers (only for developers)

  • Identify the specific code file or section with the code quality issue.
  • Investigate the issue and determine the root cause.
  • Research best practices and potential solutions for the identified issue.
  • Refactor the code to improve code quality, following the suggested solution.
  • Ensure the refactored code adheres to the project's coding standards.
  • Test the refactored code to ensure it functions as expected.
  • Update any relevant documentation, if necessary.
  • Submit a pull request with the refactored code and a description of the changes made.

Activity

  1. ABACUS-PR-BOT commented on Oct 29, 2026

    @ABACUS-PR-BOT
    Collaborator

    The proposed move is supported by the source. Three details matter for keeping it behavior-preserving:

    • Keep the shared definitions available without LCAO. Update the CPU/CUDA kernel paths and the separate ROCm library entry, plus source_cell/kernels in the legacy Makefile’s VPATH. The inspected pyabacus lists contain no affected source entries.
    • Give forwarding headers and destination headers distinct include guards. The lower-layer tests also include the old paths, so include them in the zero-old-path check.
    • Preserve get_sk’s (G+k) phase and CPU/GPU storage lifecycle. The constructor’s two PARAM reads still select CPU for LCAO; moving them unchanged preserves the global-reference budget but does not remove that configuration dependency.

    This diagnosis is based on source inspection at e5ad53046; the CPU/CUDA/ROCm build matrix and tests have not been run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    RefactorRefactor ABACUS codes

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions