Repository navigation
feat(librpa): strict 2D/3D Coulomb kernels and 2D reader-v1 producer outputs - #8176
AroundPeking wants to merge 19 commits into
Conversation
…strict-2D RI-EXX Add the strict-2D/3D Ewald Coulomb controls (ewald dimension, lambda, tail check mode with tolerances and adaptive expansion limits, direct mixed-Fourier 2D and reciprocal 3D Coulomb output selection with quadrature orders, RPA ABFS preorthogonalization, EXX V-matrix screening thresholds) to the input parameters, wire them through the EXX reading path, and extend Exx_Info_RI and the XC functional type so downstream RI modules can consume them. read_input unit tests cover the new keywords.
…metry wiring Port the strict-2D RI-EXX machinery: direct mixed-Fourier 2D and reciprocal 3D bare-Coulomb kernels, the Ewald short-range tail planner with its completeness check, Gaussian auxiliary-basis 2D integrals, MPI shell-key distribution, and the extended LRI_CV pipeline, adapted to develop's p_info_ri and timer conventions. Restore the empty-abfs construction branch in Exx_LRI::init_spencer that the signature unification dropped. The tail completeness check now bounds the projected Gaussian tensor by the RI integral pipeline's own support min(cal_V_Rcut) instead of the raw gaussianized-aux radius sum, which demanded identically-zero blocks and failed coverage in warn mode. Evaluate wigner_d factorials in double so high-L (l>=5) rotation matrices no longer overflow int and produce NaN. Wire the EXX LCAO operator, LR-TDDFT esolver, RDMFT, and center2orbital paths to the extended interfaces, and add unit tests for the kernels, tail utilities, and symmetry rotation.
…s in module_rpa Port the master_ghj output wiring that the module_rpa split dropped: the strict-2D Coulomb head normalization sidecar written next to the reader-v1 Ewald outputs, the direct mixed-Fourier 2D / reciprocal 3D Coulomb output block with its reader-v1 basis export, and the LibRPA io writers (2D head, structure symmetry, BZ sampling) plus RPA ABFS preorthogonalization, all using develop's v1 output naming. Unit tests cover the new io writers.
…cker kinds Add tests/08_RI/61_librpa_2d_reader_v1 exercising the strict-2D Ewald reader-v1 producer contract including the Coulomb head sidecar, with references generated at the producer's single-rank layout, and register it in the LibRPA producer case list. Extend check_librpa_producer.py with the strict_2d_head kind and its self-test. Regenerate the Ewald far-field coulomb_mat references of rpa_scf_gamma and rpa_scf_shrink_multik for the ported tail machinery (energies unchanged to 4e-13 eV).
…il sources Comment the construction the ported kernels implement: the positive Gram quadrature of the direct mixed-Fourier route (the tan-mapped kz rule and the Massidda-constrained Gamma plane), the analytic 2D Gaussian compensation integrals, the Ewald tail planner decision table, the Gaussian decay exponent search, and the strict-2D head normalization. The new sources carried essentially no comments, repeating the review finding from the reader-v1 producer PR. Also drop the INTERFACE OpenMP link added to the device object library during the port: this PR's OpenMP usage lives in the module_ri targets, and develop links the same device objects without publishing OpenMP, so the hunk was unrelated to this PR's scope.
The new operator_exx_nscf_cell_nearest test target referenced lcao_set_mat2d.cpp with one ../ level too many, which only surfaces when ENABLE_LIBRI=ON (the CI test configuration), breaking CMake generate. Two new headers used #pragma once, which the repository governance checker rejects; use traditional include guards, matching the guard style of the neighboring headers.
…rmat The operator cell-nearest test target never compiled under GNU CI toolchain before: its AddTest source list missed the translation units providing ModuleIO::save_sparse and hamilt::transferSerial2Parallels, and ri OBJECT-library members were duplicated on the link line. - complete the source list (hs_sparse_io, lat_r_csr, hcontainer transfer sources, center2orb, orb_read, elem_basis_idx_orb, restart) and link the ri object library instead of re-listing its members - drop tmp_mocks entries now provided by the real translation units - write the CSR fixtures in the sparse-file format the develop csrFileReader parses (SR.csr-style header with lattice block) - relax the high-L Gamma closed-form check from 2e-11 to 1e-9 relative: the degree-14 Hermite reference agrees with the kernel to a constant 2.09e-11 relative on glibc, so the old bound was knife-edge across libms
…x restart CSR The symmetry entry derived case metadata positionally, which broke when the strict-2D case joined the producer list: the generic 3D meshes tripped the nkz = 1 guard of the 2D Ewald path, and the nspin pairing appended nupdown to an unpolarized case. Derive the mesh variants and nspin from each case's INPUT instead, and iterate every listed case so the nspin = 2 coverage is kept. write_Hexxs_csr emitted the bare save_sparse layout, which read_Hexxs_csr cannot parse back. Emit the sparse-file layout shared with the DM(R) restart files (header, lattice block, CSR blocks per R) and pin the contract with a read-back unit test. List ucell_io.cpp in the two test targets that now reference UcellIO::write_ucell.
- resolve_runtime_xc_type now takes the input functional explicitly; the Exx_LRI_Interface call sites pass PARAM.inp.dft_functional from their own scope, so module_xc no longer includes parameter.h - align the Exx_Info_RI member declarations with the surrounding 4-space struct indentation
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Static review of acbe254 against ffc4d0d. No build or runtime tests were run; CI diagnostics were unavailable. The inline findings are source/analytic conclusions. Finite-mesh Gamma equivalence, compensation-tail error bounds, and sparse-restart/symmetry edge cases remain unresolved; this is not full numerical acceptance.
…, and review fixes - Extend Parallel_Reduce/Parallel_Common with sub-communicator overloads (reduce_all/max/min, reduce_max_loc, gather_int_all, bcast from an arbitrary root, my_rank/nproc/barrier) plus MPI_Type<unsigned long long>, and route the new tail reductions, direct-output drivers, eigenvector guards and raw-overlap writer through them instead of raw MPI calls. - Remove the unused symmetry_rotation_/use_spacegroup_symmetry_ members of RPA_LRI together with the now-unneeded symmetry-header dependency. - Construct the cut/full-coulomb Exx_LRI helpers with an explicit ExxLriRuntime at the remaining callers and drop the factory-delegating single-argument constructor. - Rename abf_rotation_test.cpp and nspin4_restore_test.cpp to the test_<module_name>.cpp convention and update the CMake entries. - Complete the INPUT metadata of the new registrations: strict-2D slab, k-mesh and force restrictions; tail-mode warn abort conditions on incomplete coverage or a failed Hermitian check; direct quadrature convergence notes; missing guard-shell tolerance descriptions. - Fix the dangling default argument on cal_ewald_coulomb, un-nest the ExxLriDetail void_t block that shadowed the enclosing namespace, and qualify RpaAbfsPreorthReport at the production call site in rpa_lri.cpp.
…d close gamma-dipole test link Removing the factory-delegating single-argument Exx_LRI constructor missed three call sites, so every ENABLE_LIBRI configuration failed to compile: the LR workspace in exx_workspace.cpp and both esolver_lr_init constructions. The workspace now copies the owning solver's runtime, and the esolver sites call make_exx_lri_runtime() like the interface, RDMFT and RPA paths. The new gamma-dipole unit test linked cell without the libraries its members depend on; add neighbor_search, domain_decomposition and neighbor so the mdcell_reader, record_adj and domain_decomposition symbols resolve.
|
If you have fixed specific issues, please reply to each comment. @AroundPeking |
|
Thanks for the detailed review. All inline comments have been addressed and replied to individually:
CI is green on d691c13 (16/16 checks). |
…rict2d-oct17 # Conflicts: # source/source_lcao/module_operator_lcao/test/CMakeLists.txt # source/source_lcao/module_ri/CMakeLists.txt # source/source_lcao/module_ri/module_exx_symmetry/test/CMakeLists.txt # source/source_lcao/module_rpa/CMakeLists.txt # source/source_lcao/module_rpa/module_librpa_io/unittests/CMakeLists.txt # source/source_lcao/module_rpa/unittests/CMakeLists.txt
There was a problem hiding this comment.
You added so many lines of code to this file — you must have misunderstood how the scripts in this folder are meant to be used. The purpose of this folder is to compare files: if the comparison results differ, it reports an error. This folder must not involve any physics-related knowledge. For example, given an output filea.txt , this folder's only job is to find the pre-stored reference answera.txt.ref , compare them, and report the result. That is the entire scope of this folder.
If you want to construct a lattice, that involves physics knowledge; if your output distinguishes between EXX, RI, GW, or any other physical algorithm, that's wrong — that's not what this folder is supposed to do. Please move your redundant design elsewhere.
There was a problem hiding this comment.
Fixed in bcc4bd9: the mock-data unit tests were removed from validation_tools. The folder now only holds the generic comparison tooling: check_librpa_producer.py compares each produced file against the stored reference in the case's producer_reference/ directory and reports mismatches. The 2D producer contract remains covered end-to-end by the integration case tests/08_RI/61_librpa_2d_reader_v1 (manifest-driven comparison against producer_reference/) and by the module_librpa_io unit tests, which is where the physics-aware checks belong.
There was a problem hiding this comment.
Non-blocking: Please relocate the deleted rejection cases for missing required files, incomplete strict-2D head metadata, generic nonfinite numeric data, and LRI payload mismatches to an appropriate test directory outside validation_tools. The retained wavefunction tests and C++ writer tests do not cover these parser paths. This can be a follow-up while keeping this folder within the requested comparison-only scope.
There was a problem hiding this comment.
Done in fcebd02: the rejection cases (missing required file, incomplete strict-2D head metadata, nonfinite numeric data, LRI payload mismatch) now live in tests/integrate/producer_contract/test_check_librpa_producer.py, outside validation_tools, and are registered with CTest as librpa_producer_contract.
…rict2d-oct17 # Conflicts: # source/source_io/test/CMakeLists.txt # source/source_lcao/module_operator_lcao/test/CMakeLists.txt # source/source_lcao/module_ri/unittests/CMakeLists.txt
…tools validation_tools only compares produced files against stored references. The 2D producer contract is exercised end-to-end by tests/08_RI/61_librpa_2d_reader_v1 (manifest-driven comparison against producer_reference) and by the module_librpa_io unit tests, so the mock-data unit test file is removed from this folder.
The merge resolution took the upstream version of source_io/test/CMakeLists.txt wholesale, which dropped the ucell_io.cpp entry that the Hexx restart round-trip test needs for ModuleIO::UcellIO::write_ucell.
mohanchen
added
Features Needed
ABACUS-PR-BOT
left a comment
ABACUS-PR-BOT
left a comment
There was a problem hiding this comment.
Incremental static review of 84d0923 against 370ba68. Source inspection confirms the earlier numerical fixes and most cleanup; residual source-rule items and bounded follow-ups are tracked in their existing threads. The new inline comments cover test repeatability and declaration style. No build, ABACUS/CTest run, or documentation generation was performed; CI diagnostics were unavailable. Earlier finite-mesh Gamma, compensation-tail and sparse-restart/symmetry verification limits remain open.
| // need no path special case. Exercise it with a non-default | ||
| // directory. | ||
| const std::string nondefault_directory = "nondefault_rpa_outdir/"; | ||
| require(mkdir(nondefault_directory.c_str(), 0755) == 0, |
There was a problem hiding this comment.
[P2] Make the direct-output test repeatable
The first successful run leaves nondefault_rpa_outdir/ behind, while CTest reuses this binary directory, so a second run fails at mkdir with EEXIST before testing the output. Please use an owned temporary directory with cleanup, or safely reuse and clean this fixture, and verify two consecutive runs of the selected test.
There was a problem hiding this comment.
Fixed in fcebd02: the direct-output case now owns a mkdtemp directory and removes it through an RAII guard, so consecutive CTest runs are independent. Verified locally by running the built test twice: both runs pass and no output directory is left behind.
| { | ||
| double value; | ||
| int rank; | ||
| } local{value, rank}, global{}; |
There was a problem hiding this comment.
Non-blocking: Please finish the MaxLoc type definition and declare local and global in separate statements to follow the one-variable-per-line rule. This small cleanup can be a follow-up.
There was a problem hiding this comment.
Fixed in fcebd02: MaxLoc is now a complete type definition followed by separate local and global declarations.
…iene - Route the remaining sub-communicator collectives in exx_lri.hpp and the raw-overlap writer through Parallel_Common/Parallel_Reduce, adding a gatherv_int_all wrapper, and split the reduce_max_loc locals. - Drop the default threshold on read_Cs_ao_all and pass 0.0 explicitly at the LR/BSE call sites. - Make the direct-output test own a mkdtemp directory with RAII cleanup so repeated CTest runs stay independent, and add erfcx-crossover and large-separation regressions for the scaled-erfc helpers. - State the active-only scope of rpa_abfs_preorth under shrink in the INPUT description and cover it in the preorth unit test. - Relocate the producer-checker rejection cases out of validation_tools into tests/integrate/producer_contract and register them with CTest.
Problem
rpa_scf_gamma's guard shell always fails coverage in warn mode; andSymmetry_rotation_k::wigner_doverflows int factorials for l>=5, producing NaN rotation matrices.Change
Exx_LRI::init_spencer; bound the tail check's Gaussian support bymin(cal_V_Rcut); evaluate wigner_d factorials in double.output_ewald_coulombwith v1 naming, plus the LibRPA io writers (head / structure symmetry / BZ sampling) and ABFS preorthogonalization.61_librpa_2d_reader_v1producer contract (including the head sidecar) with single-rank-layout references; regenerated Ewald far-field coulomb_mat references for rpa_scf_gamma / rpa_scf_shrink_multik (energies unchanged to 4e-13 eV); unit tests for the kernels, tail utilities, io writers, and symmetry rotation.Validation
ctest -R "MODULE_RI|MODULE_RPA|read_input|op_exx"— 24/24 pass, including thedirect 2D/3D Coulomb kernels, the Ewald tail planner and utilities, the Gaussian
2D integrals, the reader-v1 io writers, and the symmetry rotation tests.
rpa_scf_gammaandrpa_scf_shrink_multikpass with the regenerated Ewald far-field references;
bse_lr_multik,rt_tddft_pbe0_gamma, and the LibRPA producer contract (cases 60/61/64, includingthe strict-2D head sidecar) pass. The hse / lr_tddft / lr_relax failures seen
locally are LibXC-dependent cases under an
ENABLE_LIBXC=OFFbuild; CI buildswith LibXC enabled.