Repository navigation
feat(librpa): add reader-v1 producer exports - #8053
AroundPeking wants to merge 50 commits into
Conversation
…der-v1-remote-update # Conflicts: # .readthedocs.yaml # docs/conf.py
…ssion Preserve velocity_matrix.txt, rename Vxc text output and references, reject unsupported reader-v1 symmetry, abort the producer communicator on output exceptions, and validate two-rank KS assembly against same-run text data. Drop unrelated RTD changes already in develop and remove unused gauge references.
The shared MPI input fixture enables symmetry=1, so select the legacy LibRPA output version and update the corresponding parser expectation. Keep reader-v1's symmetry=-1 check and the dedicated v1 producer cases. Validated on df_iopcas_ghj with OMP_NUM_THREADS=1: - GNU 12.3 Module_IO CTest group: 53/53 passed (ENABLE_LIBRI=OFF). - Intel-built reader-v1 producer CTests: 3/3 passed, including two ranks. - Cases 60 and 64: all 46 fixture files retain their SHA256 hashes. This changes test configuration only; no user-facing INPUT behavior or generated parameter documentation changes are required.
Critsium-xy
left a comment
There was a problem hiding this comment.
Follow-up review. The file-split itself looks right now: rpa_lri.hpp is gone, the 16 replacements are real compiled units (69-433 lines, all under 500), rpa_lri_detail.h is declaration-only with an #ifndef guard, both CMakeLists.txt and Makefile.Objects are updated, agent_governance_check.py reports no errors and code_quality_score.py passes every new module_ri file. Remaining findings inline. Not built or tested locally; the Test job is still pending on this head.
Critsium-xy
left a comment
There was a problem hiding this comment.
Re-review of da7d65329. Findings 1-9, 12 and 13 from the previous round are resolved. One blocking regression in the new INPUT guard is inline below; it is what the failing Test job is reporting.
Two earlier points are still open, both in the PR description:
- The
vxc_out.dat->vxc.txtrename is a user-visible break and is not mentioned, andwrite_orb_energystill carries theterm == "vxc"special case while the other terms keep<term>_out.dat. - The k-point changes are not mentioned either:
kvec_c_fullis now the global full mesh on every rank andibz_indexis no longer overwritten by the identity map after reduction, which also fixes an out-of-bounds read inewald_vq.hpp:121. This affects EXX and has no EXX-side test.
Also: the summary still names the parameter out_librpa_reader_version instead of out_librpa_ver, and does not state the new hard requirement that out_librpa_ver = 1 needs a uniform Monkhorst-Pack grid.
Minor: esolver_fp.cpp drops from 58 to 56 in code_quality_score.py (already under the 60 line before this PR). Extracting the guard into a helper would keep LibRPA-specific validation out of the generic FP entry point.
Not built or run locally; based on static review plus the CI Test job log (run 37887346672).
There was a problem hiding this comment.
the Autotest.sh should not be involved with any physics information/algorithms, but I see libRPA appears in the script
| # echo "$has_lowf" # test out_wfc_lcao > 0 | ||
| #-------------------------------------------- | ||
| if ! test -z "$has_lowf" && [ $has_lowf == 1 ]; then | ||
| if ! test -z "$has_lowf" && [ "$has_lowf" == 1 ] && [ "${LIBRPA_PRODUCER_CONTRACT:-0}" != 1 ]; then |
There was a problem hiding this comment.
this script is only related to LCAO wave functions, the "RPA" algorithm related tags should not appear in the script
| ) | ||
| add_test( | ||
| NAME 08_RI_librpa_producer_mpi | ||
| COMMAND ${Python3_EXECUTABLE} ${ABACUS_TEST_DIR}/integrate/validation_tools/run_librpa_mpi.py |
There was a problem hiding this comment.
the CMAKE here is totally different from others, for example, the phython exe appear in the CMake, I think it would be better to be removed
|
|
esolver_fp.cpp serves as the base class, meaning it should contain the knowledge required for first-principles calculations. This class should not know about any physical quantities related to basis sets, or even orbitals. "fp" stands for first-principles. esolver_ks.cpp inherits from esolver_fp.cpp and only then acquires the concept of electronic orbitals. esolver_of.cpp, which sits parallel to esolver_ks.cpp, does not need to know anything about electronic orbitals, so esolver_fp.cpp must not contain psi in principle. The source_lcao/module_ri/rpa_lri_input.h you added should not appear here; I suggest moving it to esolver_ks_lcao.cpp instead. This is again a dependency problem: if you want to add something to k-points, it should strictly belong to k-points and have nothing to do with librpa. It is a feature added to k-points, and librpa should depend on k-points, not the other way around. Otherwise you end up with inverted dependencies — for instance, the OFDFT code would end up depending on librpa, which creates severe entanglement in the codebase. |
Summary
OUT.librpa.nspin = 2cases.out_librpa_ver(0= legacy output,1= reader-v1).out_librpa_ver = 1requires a uniform Monkhorst-Pack k-point grid and supports symmetry-reduced grids; explicit or nonuniform KPT lists are rejected before SCF.vxc_out.dattovxc.txt(and the corresponding local/EXX files tovxc_local.txt/vxc_exx.txt). This is a user-visible filename change; other orbital-energy terms retain<term>_out.dat.kvec_c_fullmesh on every rank and keep the reducedibz_indexmapping. This fixes the full-grid access inewald_vq.hpp:121and changes shared k-point semantics used by EXX; this PR has no dedicated EXX-side regression.