Fix LR optical spin weights, density restart fallback, and CSR header parsing by maki49 · Pull Request #8253 · deepmodeling/abacus-develop

maki49 · 2026-11-01T14:05:15Z

Fix LR spectra at nspin=1: remove the doubled degeneracy factor included in KS k weights at the LR/spectrum boundary. BSE already supplies spatial weights and retains its original normalization; shared k-point data is unchanged.
Fix CSR reader: support both compact STEP and cell-aware Ionic step headers with shared block parsing, and reject malformed data instead of silently misreading the values.
Fix standalone LR density input: prefer a complete chg.cube/chgs*.cube set in read_file_dir; otherwise read {suffix}-CHARGE-DENSITY.restart through the existing reciprocal-space reader and FFT every spin channel onto the real-space grid. Fail explicitly if neither source is readable. The read_file_dir input description now documents the fallback; no new input parameter is introduced.

Validation:

Optical weight tests: 5 passed, including preservation of shared KS weights and matching LR/BSE independent-transition singlet strengths.
CSR reader tests: 24 passed for both header formats and invalid-input handling.
The new RestartDensityTransformsEverySpin and MissingRestartPreservesRealDensity tests also passed with two MPI ranks and OMP_NUM_THREADS=1.

Review follow-up

Tested tree committed as 3b5e054cc0997b074e93d90aed5343117e2d5d6e:

Ignore restart modes outside the active PW cutoff even when their FFT stick belongs to the current rank; guard-buffer regression checks the former negative-index write.
Reject invalid or excess stored spin counts before modifying output buffers; retain existing supported missing-channel handling and the 2-to-4 path.
Reject non-comment tokens remaining after CSR row pointers for both header formats; trailing comments remain valid.
Exercise the production velocity-transition contraction (LR/BSE weights, distinct spin blocks, full-BSE X/Y signs) and the production collective cube-set selector (complete, partial, and absent inputs).
Clarify that the restart helper returns false for recoverable open/configuration errors but can terminate on malformed/truncated binary records.

Exact commands, run from the repository root unless noted:

cmake -S . -B /tmp/lr-density-develop-build -DBUILD_TESTING=ON -DENABLE_MPI=ON -DENABLE_LCAO=OFF -DENABLE_LIBRI=OFF -DENABLE_ELPA=OFF -DENABLE_LIBXC=OFF -DGIT_SUBMODULE=OFF -DCMAKE_BUILD_TYPE=Release
cmake --build /tmp/lr-density-develop-build --target MODULE_CHARGE_rhog_io -j 12
cmake --build /tmp/lr-density-develop-build --target MODULE_BASE_csr_reader -j 12
OMP_NUM_THREADS=1 ctest --test-dir /tmp/lr-density-develop-build -R '^MODULE_CHARGE_rhog_io$' --output-on-failure
OMP_NUM_THREADS=1 ctest --test-dir /tmp/lr-density-develop-build -R '^MODULE_BASE_csr_reader$' --output-on-failure
# MPI invocation run with /tmp as working directory:
OMP_NUM_THREADS=1 mpiexec -n 2 /tmp/lr-density-develop-build/source/source_estate/module_charge/unittests/MODULE_CHARGE_rhog_io --gtest_filter='ReadRhogTest.Restart*:ReadRhogTest.MissingRestart*:ReadRhogTest.CompleteCube*:ReadRhogTest.PartialCube*:ReadRhogTest.NeitherDensity*'
g++ -std=c++14 source/source_lcao/module_lr/test/test_spectrum_weights.cpp -lgtest_main -lgtest -pthread -o /tmp/pr8253-spectrum-tests
/tmp/pr8253-spectrum-tests
git diff --check HEAD^

Results: 18 charge tests, 28 CSR tests, 7 selected tests on each of two MPI ranks, and 7 spectrum tests passed. The two modified orchestration/production files (esolver_lr_orb.cpp, lr_spectrum_velocity.cpp) also passed mpicxx -std=c++14 -fopenmp -fsyntax-only with the existing EXX/MPI build's defines and include directories. No full executable link or end-to-end optical spectrum rerun. Cube tests cover the real source-selection routine and restart FFT, not parsing synthetic cube contents.

Governance warnings: <complex> is needed by the inline production contraction. Function contracts are documented inline; this follow-up adds no INPUT parameters or workflow options requiring separate user documentation.

Second review follow-up

Tested tree committed as 3efe8ed0ef8caf069bfd4a308e1c9d3ca295ac26. CSR frame-tail validation now continues across lines through whitespace/comments to EOF or an unconsumed next-frame header. Standalone LR restart loading requires an exact spin-count match; the shared decoder retains the existing low-level read_rhog compatibility policy for other callers. Added both-header wrapped-tail and next-frame tests, 1-to-2 rejection with unchanged buffers, and a retained complex ±G pair checking an analytic sine/cosine density while preserving the out-of-cutoff sentinel. Cube fixture paths are named once and reused.

Validation: 34 CSR tests and 19 charge tests passed; 8 selected charge tests passed on each of two MPI ranks. Exact commands (same CMake configuration as above):

cmake --build /tmp/lr-density-develop-build --target MODULE_CHARGE_rhog_io MODULE_BASE_csr_reader -j 12
OMP_NUM_THREADS=1 ctest --test-dir /tmp/lr-density-develop-build -R '^(MODULE_CHARGE_rhog_io|MODULE_BASE_csr_reader)$' --output-on-failure
# MPI test working directory: /tmp
OMP_NUM_THREADS=1 mpiexec -n 2 /tmp/lr-density-develop-build/source/source_estate/module_charge/unittests/MODULE_CHARGE_rhog_io --gtest_filter='ReadRhogTest.Restart*:ReadRhogTest.MissingRestart*:ReadRhogTest.CompleteCube*:ReadRhogTest.PartialCube*:ReadRhogTest.NeitherDensity*'

Both production syntax checks also passed. Here are the complete commands, including all defines and include paths (the existing build directory supplies generated configuration headers):

#!/bin/sh
set -eu
cd /home/fortneu/lr-grad/abacus-develop
mpicxx -DCOMMIT_INFO -DEXX_DM=3 -DEXX_H_COMM=2 -DTEST_EXX_LCAO=0 -DTEST_EXX_RADIAL=1 -DUSING_Libxc -D__ELPA -D__EXX -D__FFTW3 -D__FFT_TWO_CENTER -D__LCAO -D__LIBXC -D__MKL -D__MPI -I/home/fortneu/lr-grad/abacus-develop/source -I/home/fortneu/lr-grad/abacus-develop/source/source_base/module_container -I/home/fortneu/work/tex/lr-grad/checks/vasp-lrtddft-20261026/branch-retest-20261027/build-abacus -I/opt/intel/oneapi/mkl/2026.0/include/fftw -I/home/fortneu/LibRI/include -I/home/fortneu/LibComm/include -I/home/fortneu/soft/libxc-5.2.3-kxc/include -I/opt/intel/oneapi/mkl/2026.0/include -std=c++14 -fopenmp -fsyntax-only source/source_esolver/esolver_lr_orb.cpp
mpicxx -DCOMMIT_INFO -DEXX_DM=3 -DEXX_H_COMM=2 -DTEST_EXX_LCAO=0 -DTEST_EXX_RADIAL=1 -DUSING_Libxc -D__ELPA -D__EXX -D__FFTW3 -D__FFT_TWO_CENTER -D__LCAO -D__LIBXC -D__MKL -D__MPI -I/home/fortneu/lr-grad/abacus-develop/source -I/home/fortneu/lr-grad/abacus-develop/source/source_base/module_container -I/home/fortneu/work/tex/lr-grad/checks/vasp-lrtddft-20261026/branch-retest-20261027/build-abacus -I/opt/intel/oneapi/mkl/2026.0/include/fftw -I/home/fortneu/LibRI/include -I/home/fortneu/LibComm/include -I/home/fortneu/soft/libxc-5.2.3-kxc/include -I/opt/intel/oneapi/mkl/2026.0/include -std=c++14 -fopenmp -fsyntax-only source/source_lcao/module_lr/lr_spectrum_velocity.cpp

No full executable or spectrum rerun. The strict LR spin-count contract is documented in chg_rhog_io.h; no new INPUT parameter is introduced. The remaining governance documentation warning is covered by these API notes.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

Keep the singlet sqrt(2) amplitude factor while stripping the KS nspin=1 degeneracy from spatial integration weights. Use the corresponding spin block for polarized weights. This removes an extra factor two in velocity-gauge dipoles and four in absorption intensities. Validation: MODULE_LR_spectrum_weights CTest passed (three tests covering spin settings, nonuniform weights, and singlet versus explicit-spin intensity); git diff --check and staged governance checks passed without blockers.

Recognize the compact save_sparse format while preserving cell-aware CSR input. Validate dimensions and CSR data instead of silently loading zero-sized exchange matrices, and reject basis mismatches. Add real/complex, multi-R and malformed-input regression coverage. Validation: CSR and EXX restart CTest targets pass; Si HSE 1x1x2 SCF-to-NSCF maximum band difference is 2.20e-5 eV and all 48 LAPACK LR roots are positive. No user-input documentation change is needed: this restores reading the existing restart writer format; the reader interface comment documents compatibility.

Separate compact and cell-aware header parsing, then use one validated CSR block reader for both formats. Remove duplicated value/index loops and share comment, whitespace, empty-block and malformed-input handling. Output formats and writers remain unchanged. Validation: 24 focused reader tests pass, C++11 standalone build passes, and actual HexxR plus cell-aware matrix files read successfully. No user-facing documentation change is needed: existing file formats remain supported; interface comments describe the shared validation.

Prefer a complete cube set for standalone LR. Otherwise reuse read_rhog and transform each spin channel onto the real-space grid; report an error when neither density source is readable. Add restart FFT and missing-file tests and document read_file_dir behavior. Validation: 13 single-process charge IO tests, two new tests with two MPI ranks, and HPC integration checks for cube priority, restart/cube eigenvalue agreement, and missing-density failure.

ABACUS-PR-BOT

Please guard density-restart indices and reject leftover CSR data: the paths below can write out of bounds or silently change a matrix entry. Static review of 361ed24e; whitespace and governance checks passed with explained warnings. No build, runtime or CI verification was performed.

Non-blocking: Please add the exact commands and tested commit for the reported checks, including the MPI invocation and test selection.

ABACUS-PR-BOT · 2026-11-01T17:39:30Z

+        const std::string restart = this->in_dir + this->inp_->suffix + "-CHARGE-DENSITY.restart";
+        const Parallel::ParaWorld world = Parallel::make_pw_world();
+        const bool loaded = module_charge::read_rho_restart(restart, *this->pw_rho, this->nspin,
+            chg_gs.rhog, chg_gs.rho, world, this->ofs_running_);


Please handle restart modes that lie inside the FFT box but outside the current PW cutoff before enabling this fallback. read_rhog leaves their fftixyz2ig entries at -1, yet the owned-stick branch writes rhog[is][ig] without checking it; a higher-cutoff restart can therefore write before the buffer. Guard the mapped index and test loading onto a smaller basis.

Fixed in 3b5e054. read_rhog now writes only when the mapped index is in [0, npw). RestartSkipsModesOutsideSmallerCutoffOnOwnedStick loads G=0 plus a mode inside the FFT box but outside the smaller cutoff, checks guard values around the reciprocal buffer, and checks the constant real-space density after FFT. Passed serially and with two MPI ranks.

ABACUS-PR-BOT · 2026-11-01T17:39:30Z

+{
+    if (rho == nullptr) { return false; }
+    const bool loaded = read_rhog(filename, &rhopw, nspin, rhog, pw_world, &log);
+    if (!loaded) { return false; }


Please validate the restart's stored spin count before this call decodes into the requested channels. With nspin=1 and a readable two-spin restart, Charge::allocate provides one rhog pointer, but read_rhog loops to nspin_in and writes rhog[1]. Reject unsupported mismatches, or explicitly convert them, before the write and add a regression for this case.

Fixed in 3b5e054. The stored count is validated on the reader rank and the error is broadcast before decoding spin data or zeroing output buffers. Counts outside 1/2/4 and counts exceeding the requested channels are rejected; existing supported missing-channel behavior (including 2-to-4) is preserved. RestartRejectsExcessSpinChannelsBeforeWriting passes an exactly one-element pointer array to a two-spin restart and verifies both reciprocal and real density remain unchanged. Passed serially and with two MPI ranks.

Excess-channel rejection is now guarded, but nspin=2 standalone LR still accepts a one-channel restart. Both channels are cleared, only channel 0 is loaded, and this valence density goes straight into KernelXC, so a mismatched unpolarized restart becomes rho_up=rho_total, rho_down=0. Please reject this mismatch at the LR boundary, or define and test a valid conversion before using it; add a 1-to-2 case alongside the new reverse-direction test.

For the lower-spin restart follow-up: fixed in 3efe8ed. The LR-facing read_rho_restart requires an exact stored/requested spin count before buffers are cleared or decoded. It shares one decoder with read_rhog, whose existing compatibility behavior remains unchanged for other callers. Added the 1-to-2 case alongside the 2-to-1 regression, checking both reciprocal and real buffers remain unchanged; passed serially and with two MPI ranks.

ABACUS-PR-BOT · 2026-11-01T17:39:30Z

-
-    size_t count3 = 0;
-        while (count3 < matrixDimension + 1)
+        if (pointers.front() != 0 || pointers.back() != nnz) { invalid(); }


Please reject non-comment tokens left after the final row pointer. With dimension 2 and nnz=1, a values line 1 0, columns line 0, and pointer line 0 1 1 is accepted as values [1], columns [0], and pointers [0,0,1]; the remaining 1 is discarded and the entry moves to row 1. Check the block remainder and add this case for both headers.

Fixed in 3b5e054. After the final row pointer, the parser now allows only whitespace or a comment on the remainder of that line. Added your exact extra-value example as RejectsExtraValueShiftingRowPointers for both headers, plus a positive trailing-comment test. All 28 tests in the official CMake MODULE_BASE_csr_reader target pass.

3b5e054cc catches the original single-line case, but wrapped row pointers still bypass this check. For dimension 2 and one R block, the body below is read as values [1], columns [0], and pointers [0,0,1]: ss ends after 0 1, so the final 1 is never consumed and the entry moves to row 1. Please validate the remainder across lines while preserving allowed comments or subsequent frame headers, and cover this variant for both headers.

0 0 0 1 1 0 0 0 1 1

For the wrapped-tail follow-up: fixed in 3efe8ed. The parser now checks subsequent lines after the last CSR block, consuming only blanks/comments and leaving a following compact or cell-aware frame header untouched. Added the exact wrapped example for both headers, a trailing-data-after-comment rejection, and a two-frame regression with wrapped valid row pointers. All 34 CSR tests passed.

ABACUS-PR-BOT · 2026-11-01T17:39:31Z

+        const std::vector<double> bse_weights(nk, 1.0 / nk);
+        const std::complex<double> dipole(1.0, 0.5);
+        const auto lr_amplitude = std::sqrt(2.0) * dipole * lr_weights[0] * double(nk);
+        const auto bse_amplitude = std::sqrt(2.0) * dipole * bse_weights[0] * double(nk);


Non-blocking: Add a focused regression through the production spectrum boundary or contraction for unpolarized LR and BSE’s direct spatial weights. The helper-only tests would still pass if LR reused KS weights, BSE converted them twice, or production selected the wrong spin block.

Addressed in 3b5e054 through the production contraction option: the single-transition velocity contraction is now a small inline function actually called by LR_Spectrum, including spin * nk + ik weight selection and the X/Y sign. Tests exercise that function with converted unpolarized LR weights and direct BSE spatial weights, distinct nonuniform weights in both spin blocks, and full-BSE deexcitation terms. All 7 spectrum tests pass. This is focused production-contraction coverage, not an end-to-end ESolver/spectrum calculation.

ABACUS-PR-BOT · 2026-11-01T17:39:31Z

               std::ostream* os_warning);

+/// Decode a reciprocal-space restart and transform every spin channel into rho.
+/// Returns false when the restart cannot be read; rho is unchanged in that case.


Non-blocking: Please document that this helper returns false for recoverable open/configuration failures but can terminate on malformed or truncated data. read_rhog uses Binstream::operator>>, whose short-read path calls WARNING_QUIT, so callers cannot treat every unreadable restart as a recoverable false.

Documented in 3b5e054. The declaration now distinguishes recoverable open/configuration failures (false, real density unchanged) from malformed/truncated binary input, which can terminate through Binstream/ WARNING_QUIT. No change to the fatal short-read behavior is claimed.

ABACUS-PR-BOT · 2026-11-01T17:39:31Z

+    }
+    Parallel_Common::bcast_bool(have_cubes);
+    if (!have_cubes)
+    {


Non-blocking: Add coverage for complete cubes plus a restart, a partial spin-cube set plus a restart, and neither source. The new tests exercise read_rho_restart directly, so they do not protect the cube precedence and complete-set decision introduced here.

Addressed in 3b5e054. Extracted the actual collective cube-set decision into has_complete_charge_cubes, used directly by read_ks_chg. Tests cover complete nspin=1 and nspin=2 sets alongside a restart, a partial two-spin set falling back to the restart for both channels, and neither source (selection false and restart read false, with density unchanged). All three tests passed serially and with two MPI ranks. They test source selection and restart loading; the synthetic cube fixtures are not used as cube-parser integration tests.

Address PR deepmodeling#8253 review: guard PW indices outside the active cutoff, reject invalid or excess restart spin channels before output writes, and reject non-comment tokens after the CSR row pointers. Preserve existing supported missing-spin handling and gamma-only diagnostics. Exercise the production cube-set selection and velocity transition contraction, including LR/BSE spatial weights, distinct spin blocks, and full-BSE deexcitation signs. Document recoverable versus fatal binary read failures. The complex header is required by the inline production contraction; API contracts are documented inline, with no new INPUT behavior requiring separate user documentation. Validation: CMake MODULE_CHARGE_rhog_io (18 tests) and MODULE_BASE_csr_reader (28 tests) passed; seven restart/source-selection tests passed with two MPI ranks and OMP_NUM_THREADS=1. Seven spectrum tests passed. EXX/MPI syntax compilation passed for esolver_lr_orb.cpp and lr_spectrum_velocity.cpp. No full ABACUS spectrum rerun.

maki49 · 2026-11-02T04:01:45Z

Addressed all six inline findings from the AI review in 3b5e054 and replied individually. The PR description now includes the tested commit, exact build/test commands and MPI filter. Validation: 18 charge tests, 28 CSR tests, 7 spectrum tests, and 7 selected tests on each of two MPI ranks passed; production syntax checks passed. No full spectrum rerun is claimed.

ABACUS-PR-BOT

Static recheck of 3b5e054cc confirms the cutoff guard, excess-spin rejection, tests through production routines, and corrected failure contract. Wrapped CSR leftovers and lower-spin restarts in standalone LR remain open in their existing threads. No builds or runtime tests were run.

Non-blocking: Include the complete mpicxx syntax-check commands, with defines and include paths, so the two production-file checks can be repeated.

ABACUS-PR-BOT · 2026-11-02T06:48:44Z

+    EXPECT_TRUE(module_charge::read_rho_restart(filename, rhopw, 1, reciprocal, channels, pw_world, log));
+    EXPECT_EQ(guarded.front(), sentinel);
+    EXPECT_EQ(guarded.back(), sentinel);
+    for (double value : density) { EXPECT_NEAR(value, 0.25, 1e-12); }


Non-blocking: the restart-to-real assertions retain only G=0; the nonzero mode here is deliberately discarded. Existing reciprocal round-trip tests are useful, but a retained +G/-G pair with an analytic sinusoidal density would also check the real-space phase and record-to-grid mapping. Keep the sentinel check alongside it.

Addressed in 3efe8ed. The smaller-cutoff test now retains G=(0,0,+1) and (0,0,-1) with coefficients 0.1+0.05i and 0.1-0.05i, and checks every local grid point against rho(z)=0.25+0.2cos(2piz/Nz)-0.1sin(2piz/Nz), including the local z-plane offset. The out-of-cutoff G=(0,0,4) and both reciprocal-buffer sentinels are retained. Passed serially and with two MPI ranks.

ABACUS-PR-BOT · 2026-11-02T06:48:44Z

+    write_binary_by_hand(restart, 0, 2, miller, values);
+    if (pw_world.rank() == 0)
+    {
+        std::ofstream(directory + "chg.cube") << "cube fixture";


Non-blocking: the new fixture paths are computed directly in stream and remove arguments, contrary to AGENTS.md rule 14. Name the cube paths once per test and reuse them for creation and cleanup.

Addressed in 3efe8ed. Each fixture now declares its cube paths once and reuses the named strings for both creation and cleanup. The complete/partial/absent-source tests pass in serial and with two MPI ranks.

Reject leftover data across lines after CSR frames while preserving comments and unconsumed next-frame headers. Require exact stored/requested spin counts for read_rho_restart, sharing the decoder with read_rhog while preserving the latter legacy conversion policy. Add wrapped-tail, trailing-data and consecutive-frame regressions for both CSR headers, a 1-to-2 restart rejection preserving buffers, and a retained complex +/-G pair with an analytic sine/cosine real density alongside the cutoff sentinel. Name cube fixture paths once for creation and cleanup. API contracts are documented inline; no new INPUT options or separate user docs are needed. Validation: 34 CSR tests and 19 charge tests passed via CTest; eight selected charge tests passed with two MPI ranks, OMP_NUM_THREADS=1. Full EXX/MPI syntax commands for esolver_lr_orb.cpp and lr_spectrum_velocity.cpp passed and are recorded in the PR follow-up. No full spectrum rerun.

maki49 · 2026-11-02T10:00:12Z

Addressed the four new inline findings in 3efe8ed and replied in their respective threads. For the review-summary request, the PR description now includes the complete successful mpicxx commands with every define and include path, plus the tested commit and exact CTest/MPI selections. Validation: 34 CSR tests, 19 charge tests, and 8 selected tests on each of two MPI ranks passed; both production syntax checks passed. No full spectrum rerun.

Copilot AI balanced review requested due to automatic review settings November 1, 2026 14:05

Copilot AI reviewed Nov 1, 2026

maki49 added 3 commits November 1, 2026 09:18

maki49 force-pushed the develop branch from baf7a1b to da0d690 Compare November 1, 2026 14:18

maki49 changed the title ~~Fix a spin factor in LR spectra and a silent misreading of CSR file with header in old format~~ Fix LR optical spin weights, density restart fallback, and CSR header parsing Nov 1, 2026

ABACUS-PR-BOT reviewed Nov 1, 2026

View reviewed changes

ABACUS-PR-BOT reviewed Nov 2, 2026

View reviewed changes

Conversation

maki49 commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review follow-up

Second review follow-up

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

ABACUS-PR-BOT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maki49 commented Nov 2, 2026

Uh oh!

ABACUS-PR-BOT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maki49 commented Nov 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maki49 commented Nov 1, 2026 •

edited

Loading