symmetry-aware GW accumulation with X2C support - #15
Conversation
Use value_AO in copy_Gk_2c and get_dm_fbz; restore off-diagonal Madelung in add_Ewald. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…blocks; precompute U_q_conj
…q_lu CPU reference Hermitizes (I-P0)⁻¹ post-solve (gw_cpu_kernel.cpp:249); GPU does not. Documenting for future investigation if tol=1e-8 fails after the X2C symmetry fixes land at tol=1e-5. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds full Brillouin-zone symmetry reduction to the GPU GW path (including X2C/SOC), accumulating over IBZ k/q stars with on-the-fly rotations, and introduces an Ar X2C regression test dataset + test coverage to validate symmetry vs no-symmetry agreement.
Changes:
- Implement/standardize k- and q-space symmetry transforms for GW (including X2C), including precomputed
conj(U_q)buffers for correct complex-U handling. - Merge scalar/X2C second-τ contraction into a unified kernel path and accumulate all four X2C spinor blocks.
- Add
GW_X2C_Ar_Symmetry/HF_X2C_Ar_Symmetryregression tests and associated input/reference data.
Reviewed changes
Copilot reviewed 14 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/data/GW_X2C_Ar/make_g_input.sh | Helper script to run the Ar symmetry cases (needs portability tweaks). |
| test/data/GW_X2C_Ar/init.sh | Helper script to regenerate Ar inputs (needs portability/safety tweaks). |
| test/data/GW_X2C_Ar/atom.dat | Test structure input for Ar. |
| test/data/GW_X2C_Ar/a.dat | Lattice vectors for cubic Ar test cell. |
| test/cu_solver_test.cpp | Adds HF/GW X2C Ar symmetry regression checks and adjusts solver args. |
| src/hf_gpu_kernel.cpp | Uses k-symmetry value_AO path for X2C DM expansion and fixes Ewald block handling. |
| src/gw_gpu_kernel.cpp | Updates device memory estimate for storing both U_q and conj(U_q). |
| src/green/gpu/cugw_qkpt.h | Updates second-τ contraction API/docs (U/U_conj operands). |
| src/green/gpu/cu_symmetry.h | Adds accessors for device conj(U_q) buffers. |
| src/cugw_utils.cu | Wires U/U_conj into second-τ contraction and selects q-conjugated P buffers (one bug found). |
| src/cugw_qpt.cu | Documents known missing Hermitization in GPU LU path (TODO). |
| src/cugw_qkpt.cu | Reworks second-τ contraction GEMM chain + removes the _2C variant. |
| src/cu_symmetry.cu | Allocates and initializes device conj(U_q) buffers. |
| .github/workflows/test.yaml | Reruns failed tests with output-on-failure for easier CI debugging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 95.32% 96.70% +1.37%
==========================================
Files 13 13
Lines 877 849 -28
==========================================
- Hits 836 821 -15
+ Misses 41 28 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
After the unified second-tau kernel, the two helpers differed only by an extra G(k1_ibz) → G(k1_full) AO rotation (scalar only). Also fixes a bug where the scalar variant hardcoded need_minus_q=false, so a symmetric scalar GW with TR-active q-points would have used Pqk_tQP_ instead of Pqk_tQP_conj_. The AO transform moves to the call site under !_X2C, with a TODO to implement the X2C nso×nso G rotation on the GPU. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- cugw_qkpt: throw if exactly one of {U, U_conj} is null in
compute_second_tau_contraction (operand pair must be all-or-nothing);
tighten U parameter docstring.
- cu_symmetry: rewrite the q_p0_transforms_conj comment so it points
readers to the kernel's convention block rather than asserting a
specific OP ↔ math correspondence at the storage site — the kernel
actually uses OP_T on this buffer at GEMM 2c, which doesn't match the
"OP_N → U_q†" reading the prior comment implied.
- cu_solver_test: cast result.size() to int for MPI_Bcast count
(silent narrowing; safe in test, but flagged by -Wconversion).
- GW_X2C_Ar test data scripts: replace hard-coded \$HOME paths with
GREEN_MBPT_PYTHON / GREEN_MBPT_BIN env vars (or local ScriptDir /
GWBin overrides), with :? failure messages if neither is set.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
egull
left a comment
There was a problem hiding this comment.
Overall looks good. Have a look at the optional changes (especially the doubled memory allocations?) and then commit.
If you want to keep the code paths for double and float, we should do an if/else before the malloc/free to save memory.
Template cu_symmetry on class prec (mirroring cugw_utils<prec> and gw_qkpt<prec>) using cu_type_map<std::complex<prec>>::cuda_type for the device buffers. The five paired _d_/_f_ pointer members collapse to single cuda_complex* members, the four paired accessors collapse to two (q_p0_transform, q_p0_transform_conj), and the two transform_k_ao_device / transform_k_ao_device_2c overloads collapse to single methods. Explicit instantiation for <float> and <double> in cu_symmetry.cu. Consequences: - cugw_utils.cu drops the _sp ? _d : _f ternary at the only consumer. - cugw_utils<prec>::_cu_symmetry now holds cu_symmetry<prec>. - gw_gpu_kernel.cpp's cugw_extra budget reflects single-precision storage instead of summing both precisions. - test/cu_symmetry_test.cu uses cu_symmetry<double> for the existing double-precision roundtrip. Wrong-precision calls are now type errors at the boundary rather than runtime guards. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Turn test_symmetry_transform_roundtrip into a function template on class prec so both cu_symmetry<double> and cu_symmetry<float> are exercised end-to-end. The CPU reference (cpu_transform) stays in std::complex<double>; the helper casts to/from the device cuda_complex only at upload/download boundaries. Two REQUIREs in the Symmetry_Transform Catch2 section, both at 1e-4 (measured ~5e-5 on GPU host for both precisions on the DFT Fock test data). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Defensively symmetrize the LU solver's output to match the CPU reference's 0.5*(T + T.adjoint()) step. With P0 Hermitian (enforced above) and I-P0 Hermitian, the product is mathematically Hermitian; this suppresses float roundoff that would otherwise be amplified by the U_q rotation in the second-tau kernel. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The flags were always (void)-cast in every implementation because transform_k_ao_device handles all TR rotations on the GPU. Remove them from gw_reader1_callback and gw_reader2_callback typedefs, the four scalar/X2C lambda implementations, and both call sites. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7fe7e30 to
ffc1ba0
Compare
|
All the changes have been completed. @egull could you take a look at the relevant sections again -- I'm keeping your comments unresolved so you can see and compare. If all looks good, we can merge. |
|
The template really makes things much cleaner in this case. |
Summary
Adds full Brillouin-zone symmetry reduction to the GW GPU kernel, including the X2C (two-component spinor / SOC) path. Self-energy is now accumulated over IBZ k-stars and q-stars with on-the-fly rotation of G(k) (orbital point-group rep) and P(q) (auxiliary-basis rep), reproducing the no-symmetry result to ~1e-5 on a cubic Ar def2-svp 3×3×1 reference (9 ink no-symm → 5 ink TRS-only → 3 ink full symmetry).
Highlights
cu_symmetryorchestrating both the AO-basis k-point rotation and the auxiliary-basis q-point rotation.compute_second_tau_contraction. U_q_conj is precomputed once on device. TR branch reuses the same kernel with operand reassignment — no runtime branching in the contraction itself._2Cvariant was merged into the scalar function.GW_X2C_Ar_Symmetryruns HF+GW on cubic Ar def2-svp with no-symm / TRS-only / full-symm symmetry settings and asserts agreement per (t, k_ibz).Notes for reviewers
cugw: fix X2C cuBLAS row/col-major conventions; …) is the largest squashed unit — it contains the chronological progression of bug fixes for the row-major↔col-major confusion in the q-space transform GEMMs. Worth reviewing the final state ofcompute_second_tau_contraction(the in-source convention block explains the OP_T choices) rather than the diff itself.gw_gpu_kernel.cpp::GW_check_devices_free_spacewas verified against actual cudaMallocs incugw_qpt/cugw_qkpt/cu_symmetry; the estimate is conservative with a 20% reserve.TODOfor missing Hermitization after GETRS incompute_Pq_lu— flagging for follow-up but not blocking this PR.