Repository navigation
feat(xc): add libvdwxc-backed rVV10 nonlocal XC - #8140
CleverPhysician wants to merge 18 commits into
Conversation
|
Update after the hosted checks completed:
The PR remains Draft while the maintainer confirms the staged CPU/serial/nspin=1 scope and while the QE same-density reference comparison (including gradient/divergence sign and normalization) is completed. libvdwxc MR !32 is still a Draft dependency, so I am not presenting it as a stable release. |
Could you provide the detailed information? Another side note: libvdwxc currently requires FFTW-MPI wrapper when MPI is enabled, so be careful with MKL; for now you can consider adding a |
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static review of 32e12de74c07d2307a90c695dac6f8e452f592e5, independently cross-checked against the pinned libvdwxc implementation. Two geometry issues and one nonblocking timer-label cleanup are noted inline. No builds or runtime tests were run.
| return {a.e11 * pw.lat0, | ||
| a.e21 * pw.lat0, | ||
| a.e31 * pw.lat0, | ||
| a.e12 * pw.lat0, | ||
| a.e22 * pw.lat0, |
There was a problem hiding this comment.
[P1] Pass ABACUS lattice rows to libvdwxc
ABACUS stores lattice vectors in rows, while libvdwxc treats each C00,C10,C20 argument group as one lattice vector, so this mapping makes the convolution use the transposed cell. For the oblique lattice in the new test, mode (1,0,0) gets |G|²=1 instead of 4/3 in units of (2π/lat0)², giving the wrong nonlocal geometry. Please pass each ABACUS row consecutively and add an independent geometry check; the direct-API test currently repeats the same transpose.
| if (pw.gamma_only || pw.get_device() != "cpu" || pw.get_precision() != "double" || pw.poolnproc != 1 | ||
| || pw.nrxx != pw.nxyz || pw.nx <= 0 || pw.ny <= 0 || pw.nz <= 0 || pw.nxyz <= 0 || pw.npw <= 0 | ||
| || !std::isfinite(pw.omega) || pw.omega <= 0.0 || !std::isfinite(pw.tpiba) || pw.tpiba <= 0.0) |
There was a problem hiding this comment.
[P2] Handle left-handed cells before calling the backend
pw.omega > 0 also accepts left-handed cells: ABACUS preserves their lattice and stores abs(det), whereas the pinned libvdwxc uses the signed determinant for its integration volume. A reflected diagonal cell therefore reverses the returned nonlocal energy while the potential contraction still uses the host’s positive volume, making energy and potential inconsistent. Please reject this orientation explicitly for the staged interface or transform the cell and density consistently, and add a regression case.
| void PotNonlocalXC::cal_v_eff(const Charge* chg, const UnitCell* cell, ModuleBase::matrix& v_eff) | ||
| { | ||
| ModuleBase::TITLE("PotNonlocalXC", "cal_veff"); | ||
| ModuleBase::timer::start("PotNonlocalXC", "cal_veff"); |
There was a problem hiding this comment.
Non-blocking: The new timer labels use cal_veff, while the enclosing function is cal_v_eff. Please use cal_v_eff in both start and end to follow the timer naming rule; this has no numerical effect and can be handled in a follow-up.
|
Follow-up on head
The PR remains Draft while the new head's hosted checks are queued and the QE same-density reference is discussed. |
|
Addressed the ABACUS-PR-BOT static review on head
The intern004 adapter CTest is 1/1 after these changes, and the Si CPU/PW/nspin=1 SCF remains numerically unchanged. The PR stays Draft pending the hosted checks for the new head and maintainer review. |
| if(NOT TARGET libvdwxc::vdwxc) | ||
| message(FATAL_ERROR | ||
| "ENABLE_LIBVDWXC requires the libvdwxc::vdwxc CMake target. " | ||
| "Use the libvdwxc development build that includes the rVV10 API.") | ||
| endif() |
There was a problem hiding this comment.
That shouldn't be needed as libvdwxc's CMake config will definitely have libvdwxc::vdwxc target. Previously I did this in abacus for RapidJSON due to its embarrassing release status, but with it replaced by nlohmann-json there's no longer the case. If abacus can deal with items like dftd4 and pexsi well then the same for libvdwxc (its cmake is cleaner). If you really encountered issues please report them.
| get_target_property(LIBVDWXC_INCLUDE_DIRS libvdwxc::vdwxc INTERFACE_INCLUDE_DIRECTORIES) | ||
| if(NOT LIBVDWXC_INCLUDE_DIRS) | ||
| message(FATAL_ERROR "libvdwxc::vdwxc does not expose its include directory") | ||
| endif() | ||
| # Installed packages expose a concrete include path, while build-tree | ||
| # targets may expose generator expressions. Strip those expressions before | ||
| # probing so configuration gives a useful error instead of a later include | ||
| # failure. | ||
| set(_libvdwxc_include_hints) | ||
| foreach(_include_dir IN LISTS LIBVDWXC_INCLUDE_DIRS) | ||
| string(GENEX_STRIP "${_include_dir}" _include_dir_stripped) | ||
| if(IS_DIRECTORY "${_include_dir_stripped}") | ||
| list(APPEND _libvdwxc_include_hints "${_include_dir_stripped}") | ||
| endif() | ||
| endforeach() | ||
| unset(LIBVDWXC_HEADER_DIR CACHE) | ||
| find_path(LIBVDWXC_HEADER_DIR NAMES vdwxc.h HINTS ${_libvdwxc_include_hints} | ||
| NO_DEFAULT_PATH) | ||
| if(NOT LIBVDWXC_HEADER_DIR) | ||
| message(FATAL_ERROR "libvdwxc::vdwxc does not provide vdwxc.h") | ||
| endif() | ||
| file(READ "${LIBVDWXC_HEADER_DIR}/vdwxc.h" LIBVDWXC_HEADER_TEXT) | ||
| if(NOT LIBVDWXC_HEADER_TEXT MATCHES "VDWXC_RVV10" | ||
| OR NOT LIBVDWXC_HEADER_TEXT MATCHES "vdwxc_new" | ||
| OR NOT LIBVDWXC_HEADER_TEXT MATCHES "vdwxc_set_rvv10_parameters" | ||
| OR NOT LIBVDWXC_HEADER_TEXT MATCHES "vdwxc_set_unit_cell" | ||
| OR NOT LIBVDWXC_HEADER_TEXT MATCHES "vdwxc_init_serial" | ||
| OR NOT LIBVDWXC_HEADER_TEXT MATCHES "vdwxc_calculate" | ||
| OR NOT LIBVDWXC_HEADER_TEXT MATCHES "vdwxc_finalize") | ||
| message(FATAL_ERROR | ||
| "The selected libvdwxc headers do not provide the complete serial rVV10 API; " | ||
| "use the pinned development build containing MR !32.") | ||
| endif() |
There was a problem hiding this comment.
Looks like you add a lot of lines simply for detecting rVV10 API? For the time it might be okay, but we should drop it at some time in the future.
|
|
||
| ## Follow-up work | ||
|
|
||
| The next PRs can add a libvdwxc MPI/PFFT adapter, ordinary spin, GPU/LCAO |
There was a problem hiding this comment.
Just a side note: PFFT dependency might be dropped in the future. Actually I'm proposing a minimal dependency policy upstream, but anyways PFFT will be the first lucky one.
|
Thanks for asking. I reran the unchanged libvdwxc revision Both builds used GCC 11.2.1, Release ( With
The original MKL executable reproduces exactly the same values under a clean environment with no env -i PATH=/usr/bin:/bin LANG=C LC_ALL=C OMP_NUM_THREADS=1 MKL_NUM_THREADS=1 \
/tmp/libvdwxc-mr32-audit/build-audit/libvdwxc_rvv10_testSelecting Both configurations pass the separate CP2K-table reference comparisons in that test: energy, density/sigma derivatives, and explicit stress on a The current evidence therefore isolates the discrepancy to this MKL backend configuration. The underlying cause has not been established. Updated head This update also removes the manual CMake target/header/API checks and uses |
|
Ah OK, I can now reproduce the failure with oneMKL 2026.1.0. I will have a look into it. |
@CleverPhysician This should be fixed on libvdwxc side, please check |
|
Follow-up on
The earlier 18 successful hosted checks apply to |
|
Thanks, checked the fix at The new adapter has now also been compared against the immutable original-QE |
This branch has not been deployed
3 participants
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.Linked Issue
No linked issue. This replaces the implementation approach in closed #8031 with
a generic additive nonlocal-XC component and a thin libvdwxc adapter. It remains
Draft while the external API and first-PR scope are reviewed.
What's changed?
dft_functionalremains the semilocal functional.xc_nonlocal rvv10adds aseparate correction after the existing
PotXC; the first parameterization usesGGA_X_RPW86+GGA_C_PBE,rvv10_b 6.3, andrvv10_c 0.0093.The adapter reuses
PW_Basis,XC_Functional::grad_rho,grad_dot, chargelayouts and XC bookkeeping. The numerical backend lives in
module_vdw; thereis no second ABACUS rVV10 kernel and no rVV10-specific branch in the generic SCF
loop. It passes total density to the library, converts its derivatives from Ha
to Ry, and contracts the resulting potential with valence density for
vtxc.The library already includes the beta compensation.
Supported scope: CPU/double, full-complex PW, one process,
nspin=1, singledensity grid, right-handed cell, norm-conserving pseudopotentials, SCF energy
and potential. Other modes fail explicitly; MPI, ordinary spin, GPU/LCAO,
gamma-only, USPP/PAW, forces/stress and additional parameterizations are not
implemented by this PR.
The latest revision removes manual CMake target/header/API probes, uses the
exported
libvdwxc::vdwxctarget, adopts standardWARNING_QUITdiagnostics,removes the redundant developer page and replaces eight source-text assertions
with one executable Makefile build-report test. Python is not a new mandatory
dependency of ordinary
BUILD_TESTINGconfigurations.The ownership follow-up explicitly prohibits copying the adapter's owned
library handle. A focused host test now exercises real charge/cell storage and
checks that the correction is added without replacing existing XC values.
Unit Tests and/or Case Tests for my changes
Current verified head:
be61717cd373468c94e919096ebe9fe3e0e71eff, mergingdevelopata2b8bc30f3f5288bbef9a01fa2d5f188269cf2af. The only manual mergeresolution retains the upstream mapped-density comment beside all rVV10 INPUT
fields; both parents' additions/deletions are preserved. No adapter algorithm
change was made. Its exact source tree
1b801998eb5dc8b3eb8f1a2521fbb32766d864ebwas archived, built and tested before creating the merge commit.
The complete serial PW executable, both C++ test targets, all three
LIBVDWXC CTests and a fresh Si SCF now pass with the fixed standalone-FFTW
libvdwxc revision
94410ebdf3dc9615a40f40c3a5b59dbe20a433d2. This remains theunreleased Draft MR !32, not a released rVV10 API. The earlier complete
build used its parent
91ef4820...; those old results are not used as evidencefor this new dependency build.
Current toolchain: GCC 11.5.0, CMake 3.26.5, Libxc 6.2.2, OpenBLAS 0.3.34,
standalone FFTW 3.3.11, CPU/serial/OpenMP-off. CMake's testing configuration
selects C++14. Actual runtime DSO paths and hashes match the selected library;
there is no MKL FFT provider in the process. The binary SHA256 is
620fbe9d54ec15896ddc163c4aac87626bdfbc91bff14b0b90b485af87e2aec5.The archive build reports Git Commit N/A; its external tree identity is recorded,
not inferred from that version string.
Executed in intern004 Slurm jobs 556238 (build/tests) and 556243 (SCF):
cases, one real host case, and the Makefile build-report test.
6.95559e-7 < scf_thr=1e-6, total energy-208.0135469216236 eV.The log records eight
PotNonlocalXC::cal_v_effcalls. Actual DSO hashing,explicit convergence/final-energy markers, finite residuals, CLI success and
seven normal/seven Python-
-Overifier tests passed. No new energy tolerancerelative to the old run was introduced. No additional QE run was required.
finite-difference energy/potential check and independent reciprocal geometry
for equivalent oblique/orthogonal cells. Reintroducing the wrong cell
transpose makes the geometry test fail (
1.22e-4 Ryenergy discrepancy);restoring the correct mapping passes.
passes afterward. The host test checks valence-plus-core input, preservation
of prior nonzero energy/potential, valence-only
vtxcand unchanged inputarrays. Deliberately omitting core density makes it fail (
3.33e-5 Ryenergydiscrepancy); replacing additive updates with assignment also fails.
All mutation copies were restored before final tests and builds.
density length and nonfinite density.
c3dedfb/parent-dependency coverage: two-atom Si SCF converged in seven iterations withE_total = -208.0135469216242 eVandE_xc = -4.2517234318 Ry.The matching
xc_nonlocal nonecase also converged. Both passed--check-input; double-grid andnspin=2negative cases exited with code 1at the intended parser diagnostic. The pseudopotential is the repository's
source/source_estate/test/support/Si_ONCV_PBE-1.0.upf,ecutwfc=20 Ry,Gamma k-point and
scf_thr=1e-6.ENABLE_LIBVDWXC=OFF,BUILD_TESTING=ON,CMAKE_DISABLE_FIND_PACKAGE_Python3=ONconfiguration andnonlocal_xctarget build passed. This is a default-path smoke check, not a second full
executable build.
Local checks:
python3 -m unittest discover -s tests/rvv10 -p 'test_*.py' python3 docs/generate_input_main.py /tmp/rvv10-final-parameters-oct14.yaml python3 tools/03_code_analysis/agent_governance_check.py --base origin/develop --head HEAD --format text git diff --check origin/develop...HEADResults: Python 1/1; INPUT reference generated from the new executable's
metadata; no whitespace errors; governance has no blocking findings (only
header-dependency review warnings). Generated INPUT documents remain untracked
following the latest upstream documentation change.
Dependency follow-up: after Growl's FFT-boundary fix, revision
94410eb...passes 8/8 CTests with standalone FFTW and 8/8 with MKL
2022.2.1/lp64/sequential/dynamic (GCC 11.2.1). Both include
fft.weightsand
rvv10.serial. The seven ABACUS adapter cases and one real host casealso pass with the new standalone-FFTW library. These runs used Slurm job
555743 on intern004's
test001; FFT providers were kept separate.The earlier 69 failing comparisons describe the retained parent-revision
MKL result, not the updated dependency. This is not an MKL ABACUS validation.
Independent fixed-QE comparison now reads the unchanged saved density and
original QE output (QE
b3af0d0e810a611c455fd80fca6ee523dfaaee95). Sevencases use an 8x10x12 grid and a 10x12.5x15 Bohr cell, comparing energy,
valence
vtxc, all 960 potential values and nine directional steps per case.The maximum energy difference is
3.50e-13 Ry; the current-adapterenergy/potential directional residual is at most
1.25e-10 Ryath=1e-4after normalization by the direction's integrated absolute value.
Pointwise potential equivalence is not established: the ordinary
valence/core case differs by
2.49e-6 Ry; low-q and small-gradient casesdiffer by up to
7.71e-2 Ry. Original QE retains derivatives after qminclipping and zeros the sigma derivative at
sigma<=1e-12, whereas the libraryuses the derivative of its clipped/analytic energy. Their own-energy and
QE-energy directional checks are reported separately. Also, 245 host PW
modes describe the gradient/divergence cutoff, not identical convolution
cutoffs: QE uses its spherical PW set; libvdwxc uses the full FFT grid.
Both dependency revisions produced byte-identical output for these orthogonal
fixtures. No reference output was modified and no global pass verdict is assigned.
Material follow-up on the unchanged implementation (Slurm 555799): full archived
20/30/40 Bohr graphene densities give energy differences of 1.22e-15,
4.16e-16 and -5.45e-15 Ry relative to the original QE output. Raw pointwise
potential differences remain 1.499/1.603/0.657 Ry; density-weighted RMS is
0.00771/0.00631/0.00759 Ry. Most of this survives projection onto the exact
host PW set, so the reference's out-of-PW divergence terms are not a sufficient
explanation. All 27 input hashes were checked; the negative tail sample is retained.
Eight retained/excluded/Nyquist direction cases have candidate-self residuals
at most 5.80e-12 Ry at h=1e-4 (L1 normalized). The actual explicit-grid cutoff
is recorded; a 20 Ry request in the small oblique cell is clamped to 3.553 Ry.
Real-material self differences (Slurm 555816) separately use whole, fixed dense,
and fixed positive-tail directions, retaining all five steps. At h=1e-4, whole
and dense residuals are at most 7.37e-12 and 8.87e-10 Ry. Tail directions have
small L1 and develop small-step cancellation noise; all raw residuals remain
reported, with no overall accuracy verdict or tolerance relaxation. Independent
reconstruction confirms their baseline potential contractions and direction norms.
These are candidate-self derivatives, not original-QE energy derivatives.
Original-QE material energy differences have now been computed for all 45 prescribed
perturbations without modifying its numerical module (Slurm 555845). The first
40 Bohr baseline reproduces energy, vtxc and every potential value exactly; tiny
20/30 Bohr baseline drifts remain recorded and fail the unchanged exact-baseline
check. Only the first 40 Bohr case is admitted to the strict partial comparison.
Its perturbed energies differ by at most 6.62e-15 Ry; at h=1e-4, QE-energy versus
candidate-potential residuals are 7.65e-12 Ry for the whole-density direction and
8.98e-10 Ry for the dense direction (L1 normalized). Original-QE self residuals
are 4.20e-8 and 4.48e-7 Ry respectively; its positive-tail self residual remains
about 0.004469 Ry over all five steps. All steps and both potentials are retained;
this supports these tested derivatives, not general pointwise equivalence.
Repeat/allocation controls (Slurm 555852) also show small potential changes from
the same QE executable, up to 2.18e-9 Ry relative to its first 40 Bohr baseline,
with energy-FD variation at most 3.82e-13 Ry. This does not establish FFT planning
as the cause or justify changing the exact-baseline criterion. Twelve focused
analysis tests pass normally and with Python -O. The subsequent single-branch diagnostic is described below; no production
change or global numerical pass is claimed.
Separately named diagnostic-only QE modules isolate two numerical conventions,
without changing the saved original reference or the adapter. B restores only
the small-sigma gradient derivative: compiled scalar probes first reproduce
the original discrepancy, then ten witnesses agree with independent
80/100-digit Decimal differences while density/zero-gradient/high-sigma
controls stay unchanged. The original kernel, energy, q0 and beta are retained.
A B observer streams the actual three H fields. Independent full-FFT
reconstruction reproduces that run within 3.33e-16 Ry and identifies the
unmodified out-of-PW H coefficients in its divergence buffer. This observer
differs from the first B baseline by up to 2.21e-8 Ry; both remain separate,
with no selection, reference replacement or asserted cause for this drift.
The existing QE fftx_oned2threed helper was subsequently tested through the
real FFT path: the original mixed/outside/Nyquist cases fail the predeclared
analytic check, while independent save/clear/refill passes all five cases.
Clear-without-refill and wrong-index mutants fail; all 245 saved coefficients
are retained exactly and all 715 excluded entries become zero.
Full C-only and B+C modules have now actually run once each on the archived
40 Bohr density (Slurm 556049, 110592 points, requested/actual 160 Ry,
25709 host G, serial/non-Gamma). C-only adds only independent h_g storage
and the existing clear/refill helper after fwfft; its q0/sigma branch stays
original. B+C derives separately from B. Complete inverse source patches
restore their respective parents; iG, signs, inverse FFTs, kernel and energy
bodies are unchanged. C minus original is confined to out-of-PW coefficients
(PW-inside maximum 5.14e-17 Ry), but C versus adapter still differs by
0.656 Ry. B+C versus the current adapter has raw full-grid maximum
1.87e-8 Ry, density-weighted RMS 2.69e-11 Ry, energy difference
5.45e-15 Ry and vtxc difference -8.47e-16 Ry, without offset fitting.
B+C versus first B has a 2.05e-8 Ry PW-inside cross-run difference; B+C
versus the earlier H-based algebraic prediction differs by 2.70e-8 Ry raw,
reported descriptively rather than forced to zero.
All fifteen saved original-QE and candidate E+/E- steps are retained.
At h=1e-4, original-QE energy differences versus B+C potential contractions
have whole/dense normalized residuals 7.65e-12/8.98e-10 Ry; all five
positive-tail residuals remain reported (6.59e-8 to 8.18e-7 Ry with small-L1
cancellation noise). These are not newly computed C/B+C self energy
differences. Seven new analysis tests pass normally and with Python -O;
an independent raw-array/reciprocal-cell/component-FFT crosscheck confirms
all fifteen records and the unshifted full-grid statistics.
This single-density diagnostic supports the two-convention explanation;
it is not an unmodified-QE pointwise pass, a resolution of every low-q case,
or a global accuracy verdict. Those diagnostics did not change the adapter.
The uniform lower-q witness is now checked independently. For saved case 2,
all 960 points and all 18 perturbed endpoints stay inside the q floor, with
zero gradient. Thus theta is proportional to n^(3/4) at fixed qmin and the
fixed-kernel energy has E=betaN+AVn^(3/2), giving
v=beta+1.5(E-beta*N)/N. Using each source's own energy, the candidate's
full-grid potential differs from this relation by 2.43e-16 Ry maximum;
the saved original-QE potential differs by 1.90e-4 Ry. All nine saved
energy-difference steps, original potentials and accumulated norms remain
separate; the uniform G0 case does not test the differing convolution cutoffs.
A further diagnostic-only A probe changes only dq0_dq to zero inside the
original q0<qmin branch, without B/C changes or removal of the weight
derivative. Archived actual-original scalar output first fails the new
80/100-digit clamped-scalar central-difference check (Slurm 556090).
Only afterward was A derived and compiled; Slurm 556093 evaluates ten
scalar witnesses once: floor_zero dq/dn changes from 1.861687e5 to zero,
all ten q0/chi values and the other nine density derivatives remain
bitwise unchanged. Thirteen focused analysis tests pass normally and with
Python -O, with independent raw-index/Decimal crosschecks. The main
diagnostic API stays private; the separately exposed probe has the same
q0 routine body. No original QE rerun, new E+/E-, full A energy/potential,
nonzero-gradient floor-chi, unclamped UEG cancellation, or global numerical
pass is claimed. Original references and production sources remain unchanged.
Checks not run: independent SCF reference comparisons;
MPI/spin/force/stress validation; the complete local default test suite.
The former
95528a1...head had successful hosted checks, but that result doesnot certify the new merge. At 2026-10-15 02:12:38 UTC, head
be61717...is Draftand conflict-free; its CI/doc builds are running (one check successful, twelve
in progress, two queued; both doc statuses pending). Default upstream CI does
not build the optional unreleased libvdwxc backend. The complete backend build
and SCF evidence above is separate from hosted CI.
Governance Notes
headers declare the stored
unique_ptr,vector,arrayand string types.The concrete adapter needs libvdwxc's
vdwxc_datadeclaration; its documentedcomplex.hpre-include avoids C-linkage parsing problems. The host stillforward-declares the numerical backend rather than including its C API.
xc_nonlocal,rvv10_b,rvv10_cmetadata, installationinstructions and VV10/rVV10 citations.
xc_nonlocal nonepreserves existingbehavior. Pairwise D2/D3/D4 combinations are rejected.
behavior; no additional user-facing documentation is needed. The test README
includes the new host target and what it checks.
Potentialreceives the selection explicitly and registers theadditive component after semilocal XC. Its numerical work stays behind the
module_vdwinterface; existing solvers, operators and DFT-D code are notreplaced.
nspin=1SCF first PR acceptablewith MPI and ordinary spin in follow-ups? Earlier review of feat: add restricted rVV10 plane-wave support #8031 expected
those capabilities; I have not treated the staged scope as approved. The PR
stays Draft pending this decision, the external API review, independent
reference validation.