Refactor projection in pw basis - #8007
Merged
Merged
Conversation
added 6 commits
September 23, 2026 16:01
Create an empty source_pw/module_proj OBJECT library and wire it into the build (add_subdirectory before module_pwdft; final link after module_pwdft). A placeholder .cpp keeps the target valid until the first real source moves in. No functional change; full CPU build and governance check pass.
Migrate RadialProjection::RadialProjector (SBT radial projector) from module_pwdft to the new module_proj. Pure file relocation: update include paths in onsite_proj.h and radial_proj_test.cpp, rewire both CMakeLists and the Makefile.Objects VPATH. Remove the placeholder source. No behavior or interface change, so no docs/parameters update is required. Verified: full CPU build links, MODULE_PW_radial_proj_test passes.
Migrate the onsite projector print helpers (print_orb_chg, print_mag_table, print_proj_status) from module_pwdft to module_proj. Pure relocation: update the three include paths and both CMakeLists. Makefile.Objects needs no change since module_proj is already on the VPATH. No unit test exists for these print helpers; no behavior or interface change, so no docs update is required. Verified: full CPU build links.
Migrate the onsite projector computation backend (cal_becp / cal_dbecp /
cal_force / cal_stress, six files) from module_pwdft to module_proj. Update
the self/nonlocal_maths includes to full paths and rewire both CMakeLists.
Onsite_Proj_tools still includes module_pwdft headers (vnl_pw.h, kernels/
{force,stress}_op.h); this temporary reverse dependency is tolerated and
recorded in module_proj/CMakeLists.txt. kernels/onsite_op stays in pwdft
since it is used by op_pw_proj.cpp, not by the tools. nonlocal_maths.hpp
also stays in pwdft (shared by seven files). OBJECT libraries link fine.
No behavior or interface change; verified by full CPU build linking.
Migrate the OnsiteProjector class (onsite_proj.h/.cpp, onsite_proj_init.cpp, onsite_proj_overlap.cpp) from module_pwdft to module_proj, and update the nine include sites across lcao/io/pwdft. The DFT+U/DeltaSpin force & stress member functions (onsite_proj_force_stress.cpp) stay in module_pwdft since they depend on Plus_U_Base; the class declaration now lives in module_proj while those definitions remain in pwdft, linked via the OBJECT libraries. The singleton and public API are unchanged. No behavior or interface change, so no docs update is required. Verified: full CPU build links, and the deltaspin ctest set (5 tests incl. deltaspin_pw_test) passes.
The commented-out STAGE 1/STAGE 2 body of OnsiteProjector::tabulate_atomic is obsolete: the actual tabulation is done by Onsite_Proj_tools. Remove the dead block and keep only the k-point dimension bookkeeping, with a comment recording where the real work happens. No behavior change; full CPU build links.
mohanchen
added
Refactor
The per-projector 2x2 occupation block accumulation
rho^{ss'} = sum_i w_i conj(proj^s) proj^{s'}
was previously inlined in OnsiteProjector::cal_occupations and
duplicated in DeltaSpin's accumulate_Mi_from_becp. Extract it as a
free function elecstate::occ_from_proj in source_estate/occ_comput so
both call sites can share it; this commit only adds the function and
its unit tests, call-site migration follows in later commits.
Verification: ctest -R MODULE_ESTATE_occ_comput passes (5 cases).
No INPUT parameter change, docs not required.
Replace the inline occupation accumulation loop in OnsiteProjector::cal_occupations with the shared free function elecstate::occ_from_proj extracted in the previous commit. The behavior is identical: same weights, same spin-channel placement (isk for nspin=2), same even split for nspin=1. The local variables proj_p, wg_ik, isk and nat are computed before the call so no conditional expression appears in the argument list. Verification: module_proj target builds; numerical equivalence is covered by the oracle-based unit tests in test_occ_comput.cpp. Runtime check pending on a full abacus build. No INPUT parameter change, docs not required.
Rebuild spinconstrain::accumulate_Mi_from_becp on the shared core elecstate::occ_from_proj instead of its own inline becp loops. The public signature (including spin_sign) is unchanged, so call sites in deltaspin_pw_mi.cpp need no modification. Internally the function now computes the per-projector 2x2 occupation blocks once, then aggregates them into per-atom magnetic moments: npol=2 (nspin=4): Mi = sum_iprj pauli_to_moment(block_iprj) npol=1 (nspin=2): Mz = sum_iprj (occ[0] - occ[3]) == weight*occ*spin_sign spin_sign is mapped to isk (+1 -> 0, -1 -> 1) for the shared core; nspin=1 never occurs in DeltaSpin. Two new unit tests (RealFunction_Npol1/Npol2_MatchesOracle) call the real function and require agreement with the pre-refactor oracle loops. deltaspin_core_test now links mi_tools.cpp and occ_comput.cpp. Verification: ctest -R deltaspin_core_test passes (24 cases, incl. the 2 new ones). No INPUT parameter change, docs not required.
Record coding rule 14 in AGENTS.md: do not write conditional or computed expressions in a function's argument list; assign to a named local variable first and pass that variable. This was agreed during the occ_from_proj refactor (commits 34e7c5cb9, 1e381236f) and is applied in those call sites.
The Makefile build was missing occ_comput.o, causing undefined references to elecstate::occ_from_proj from onsite_proj_overlap.o and mi_tools.o at link time. The CMake build already includes it.
…te tests - Convert _build_backward_map, _build_forward_map, _build_sbt_tab (type-wise overload), and _mask_func from RadialProjector static members to RadialProjection namespace free functions. - Remove unused sbfft() declaration and _do_mask_on_radial empty implementation along with ~100 lines of commented-out legacy code. - Slim radial_proj.h by dropping unitcell.h and pw_basis_k.h includes; add matrix.h and <tuple> for free function signatures. - Update onsite_proj_init.cpp call sites to the new free function API. - Remove dead RadialProjector rp_ member from OnsiteProjector and drop the now-unneeded radial_proj.h include from onsite_proj.h. - Migrate radial_proj unit test from module_pwdft/test to module_proj/unittests, renamed to test_radial_proj.cpp per governance naming rules, with API calls updated. - Register the test in module_proj/unittests/CMakeLists.txt under BUILD_TESTING and remove the old registration from module_pwdft/test/CMakeLists.txt. Verification: python3 tools/03_code_analysis/agent_governance_check.py --staged (no findings). Compile and runtime tests not run per instruction.
- Remove OnsiteProjector::read_abacus_orb member function and its explicit instantiations; the implementation duplicated the existing ModuleIO::read_abacus_orb in source_base/module_out/orb_io.h. - Update init_proj to call ModuleIO::read_abacus_orb directly. - Drop the now-unneeded parallel_common.h include from onsite_proj_init.cpp. Verification: python3 tools/03_code_analysis/agent_governance_check.py --staged (no findings). Compile and runtime tests not run per instruction.
- Delete ~40 lines of commented-out legacy gemm implementation in overlap_proj_psi. - Delete debug std::cout comment lines in cal_occupations. Verification: python3 tools/03_code_analysis/agent_governance_check.py --staged (no findings). Compile and runtime tests not run.
…lementwise The caller passed &(gcar[ik*npwk_max].x) and transfer_gcar copied it via gcar_tmp.assign(gcar_in, gcar_in + 3*npw_max), which assumes Vector3<double> is a contiguous 3-double POD. The standard does not guarantee the memory layout of Vector3, so this was undefined behavior (as noted in the code comments). Change transfer_gcar to take const ModuleBase::Vector3<FPTYPE>* and unpack x/y/z elementwise into the contiguous buffer. This makes the copy well defined regardless of Vector3's internal layout.
- radial_proj.h: include source_base/realarray.h for ModuleBase::realArray - radial_proj.cpp: include <algorithm> for std::max_element/transform/for_each - onsite_proj_init.cpp: include radial_proj.h for RadialProjection namespace These were previously satisfied via transitive includes that were removed during header dependency cleanup.
AsTonyshment
approved these changes
Sep 25, 2026
AsTonyshment
left a comment
AsTonyshment
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, I found no significant correctness issues in this refactor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor projection in pw basis