Skip to content

Fix GPU spinor density accumulation without magnetization - #7902

Merged
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:fix/issue-7549-preserve-gpu-charge
Sep 3, 2026
Merged

Fix GPU spinor density accumulation without magnetization#7902
mohanchen merged 1 commit into
deepmodeling:developfrom
Critsium-xy:fix/issue-7549-preserve-gpu-charge

Conversation

@Critsium-xy

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

Fix #7549

Issue validation

The kernel-level problem is reproducible on the current develop commit c2fa3ac19539bd10817d66e99ac5ff1cfd2156dd with an NVIDIA A800 and CUDA 12.9.

A direct call to the GPU spinor-density kernel with DOMAG=false and DOMAG_Z=false produced:

rho0 = 0 0; rho1-3 = 0 0 0 0 0 0

For the same initial density and wavefunctions, the CPU implementation preserves and accumulates scalar charge, giving rho[0] = {2.0, 8.5}. The regression test added by this PR failed before the production fix with those exact CPU/GPU differences and passes afterward.

The standard INPUT initialization currently makes this boolean combination unreachable for nspin=4: noncolin=true selects (DOMAG, DOMAG_Z)=(true,false), while noncolin=false selects (false,true). Therefore this PR fixes a verified backend contract inconsistency and protects future/internal callers; it does not claim that a normal current INPUT case already reaches the affected branch.

Unit Tests and/or Case Tests for my changes

  • Commands run:
    • PATH=/usr/local/cuda-12.9/bin:$PATH cmake -S . -B build-cuda-issue7549 -G Ninja -DUSE_CUDA=ON -DBUILD_TESTING=ON -DENABLE_FLOAT_FFTW=ON -DCMAKE_CUDA_ARCHITECTURES=80 -DFETCHCONTENT_FULLY_DISCONNECTED=ON
    • PATH=/usr/local/cuda-12.9/bin:$PATH cmake --build build-cuda-issue7549 --target MODULE_ESTATE_Elecstate_Op_UTs -j2
    • OMP_NUM_THREADS=1 ctest --test-dir build-cuda-issue7549 -V -R '^MODULE_ESTATE_Elecstate_Op_UTs$'
    • PATH=/usr/local/cuda-12.9/bin:$PATH cmake --build build-cuda-issue7549 --target abacus_basic_gpu -j4
    • git diff --check
    • python3 tools/03_code_analysis/agent_governance_check.py --staged
    • python3 tools/03_code_analysis/code_quality_score.py --output /dev/stdout source/source_estate/kernels/test/elecstate_op_test.cpp
  • Result summary:
    • The focused CUDA regression failed before the production fix: GPU rho[0] was {0, 0} while CPU rho[0] was {2.0, 8.5}.
    • After the fix, all 5 tests in MODULE_ESTATE_Elecstate_Op_UTs passed on an NVIDIA A800.
    • The complete CUDA abacus_basic_gpu executable built successfully.
    • git diff --check passed.
    • The changed C++ test file received a code-quality score of 86/100, above the required score of 60.
    • The governance checker completed with only the expected documentation-sync warning addressed below.
  • Checks not run, with reason:
    • ROCm runtime tests were not run because no ROCm compiler/device is available in the environment. The HIP change mirrors the verified CUDA one-line correction.
    • The full integration suite was not run because the affected behavior is isolated to the density-accumulation kernel and is directly covered by the focused CPU/GPU parity test.
    • pre-commit was not run because it is not installed in this environment.

What's changed?

  • Preserve scalar charge density rho[0] in the CUDA and ROCm spinor-density kernels when magnetization output is disabled.
  • Continue clearing only rho[1..3], matching the CPU implementation.
  • Add a focused CPU/GPU parity regression test for the affected boolean combination.
  • Enable the existing GPU portions of the elecstate operator unit-test target in CUDA/ROCm test builds and update their stale memory-release calls to the current API.

Governance Notes

  • INPUT/docs changes: none. No INPUT keyword, default, validation, file format, or documented user workflow changes.
  • Core module impact: limited to the source_estate CUDA/ROCm density-accumulation kernel and its focused unit-test target. No interface, global dependency, header dependency, or MPI behavior changes.
  • Exceptions requested: none.

@mohanchen mohanchen added Bugs Bugs that only solvable with sufficient knowledge of DFT Refactor Refactor ABACUS codes collinear/non-collinear/SOC/delta-spin Issues related to SOC labels Sep 3, 2026

@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

Copy link
Copy Markdown
Collaborator

We should have tests to protect these features.

@mohanchen
mohanchen merged commit cc06269 into deepmodeling:develop Sep 3, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugs Bugs that only solvable with sufficient knowledge of DFT collinear/non-collinear/SOC/delta-spin Issues related to SOC Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] Preserve total charge in GPU noncollinear density accumulation

2 participants