Refactor: inject nlocal/nbands into the LCAO dense diagonalizers - #7721
Draft
Critsium-xy wants to merge 1 commit into
Draft
Refactor: inject nlocal/nbands into the LCAO dense diagonalizers#7721Critsium-xy wants to merge 1 commit into
Critsium-xy wants to merge 1 commit into
Conversation
Group D of the source_hsolver PARAM removal, and the largest single cluster: the six dense LCAO diagonalizers all sized their eigenvalue buffers from PARAM.globalv.nlocal and copied out PARAM.inp.nbands eigenvalues. Each of DiagoLapack, DiagoScalapack, DiagoElpa, DiagoElpaNative, DiagoCusolver and DiagoCusolverMP now takes (nlocal, nbands) through its constructor and stores them as members. DiagoElpaNative additionally takes use_gpu, replacing its PARAM.inp.device == "gpu" test. HSolverLCAO gains nbands and use_gpu (it already received nlocal in deepmodeling#7711) and supplies all eleven construction sites across hamiltSolvePsiK, parakSolve and parakSolve_cusolver. The three call contexts agree on both values: parakSolve distributes the same nlocal x nlocal matrix over a smaller pool grid, and parakSolve_cusolver gathers it per k-point, so a single pair covers all of them. nbands is injected rather than read from ParaV->get_nbands(). The two are equal for the LCAO path (LCAO_init_basis.cpp seeds ParaV from PARAM.inp.nbands), but module_lr sets paraMat_.nbands to nocc + nvirt, so sourcing it from ParaV would plant a trap for any future caller. The existing ParaV->get_nbands() reads inside parakSolve are left alone; they are not PARAM reads and are out of scope. In diago_scalapack.cpp, four call sites pass the address of the value to the Fortran routines. PARAM.globalv returns a const reference, so &PARAM.globalv.nlocal was already a const int*, matching pdsygvx_/pzhegvx_'s const int* n; &this->nlocal has the same type. Also resolves the two error-message strings in diago_lapack.cpp and diago_scalapack.cpp that embedded "PARAM.inp.nbands = ". These were deferred from deepmodeling#7706 because removing them needed exactly the nbands member added here. Two dead commented-out ELPA_Solver calls in diago_elpa.cpp are removed rather than updated; each duplicated the live statement two lines below it. The three LCAO diagonalizer tests supply their own nlocal/nbands, which they already read from the H/S input files, so their PARAM.sys.nlocal / PARAM.input.nbands writes and the now-dead `#define private public` include blocks are dropped. PARAM occurrences in source_hsolver production code: 60 -> 5. What remains is DiagoIterAssist's basis_type/calculation switches (group E) and one comment in simple_pexsi.cpp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Critsium-xy
marked this pull request as draft
July 31, 2026 08:18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Follow-up to #7706 and #7711, continuing the removal of
PARAM.*reads fromsource_hsolver(coding rule 1 inAGENTS.md).This is the largest single cluster: the six dense LCAO diagonalizers all sized their eigenvalue buffers from
PARAM.globalv.nlocaland copied outPARAM.inp.nbandseigenvalues — 55 reads across six files.Changes
DiagoLapack,DiagoScalapack,DiagoElpa,DiagoElpaNative,DiagoCusolverandDiagoCusolverMPeach take(nlocal, nbands)through the constructor and store them as members.DiagoElpaNativeadditionally takesuse_gpu, replacing itsPARAM.inp.device == "gpu"test.HSolverLCAOgainsnbandsanduse_gpu(it already receivednlocalin #7711) and supplies all eleven construction sites acrosshamiltSolvePsiK,parakSolveandparakSolve_cusolver.The three call contexts agree on both values
Worth confirming during review, since it is what lets one
(nlocal, nbands)pair serve all of them:hamiltSolvePsiK(kpar_lcao == 1) uses the globalParaVdirectly.parakSolve(kpar_lcao > 1) goes throughdiag_pool, which receives pool-local matrix blocks — but the distributed matrix is stillnlocal x nlocal, just spread over a smaller grid. The function already feedsParaV->get_global_row_size()intok2d.set_para_env.parakSolve_cusolvergathers the matrix per k-point withnrow = ncol = ParaV->get_global_row_size().Why
nbandsis injected rather than read fromParaVHSolverLCAOalready holdsParaV, andParaV->get_nbands()does equalPARAM.inp.nbandson the LCAO path (LCAO_init_basis.cppseeds it from there). Butmodule_lrsetsparaMat_.nbandstonocc + nvirt, so sourcing it fromParaVwould plant a trap for any future caller that reaches these diagonalizers through a differently-initialisedParallel_Orbitals. It is a latent hazard rather than a live bug today —module_lrdoes not use these classes — but injection costs nothing and closes it.The existing
ParaV->get_nbands()reads insideparakSolveare deliberately left alone: they are notPARAMreads and are out of scope here.Address-of sites in ScaLAPACK
Four call sites pass the address of the value to the Fortran routines rather than the value.
PARAM.globalvreturns a const reference, so&PARAM.globalv.nlocalwas already aconst int*, which matchespdsygvx_/pzhegvx_'sconst int* n;&this->nlocalhas the same type. This was type-checked against the real prototypes (see Testing).Incidental cleanups in code that had to be touched anyway
diago_lapack.cppanddiago_scalapack.cppthat embedded"PARAM.inp.nbands = ". These were explicitly deferred from Refactor: start removing PARAM from source_hsolver (hsolver.cpp + redundant reads) #7706 because removing them required exactly thenbandsmember added here.ELPA_Solvercalls indiago_elpa.cpp; each duplicated the live statement two lines below it.Tests
The three LCAO diagonalizer tests already read
nlocal/nbandsfrom their H/S input files, so they now pass those directly. TheirPARAM.sys.nlocal/PARAM.input.nbandswrites are dropped, along with the#define private public/#include parameter.h/#undef privateblocks that existed solely to enable those writes.Effect
PARAM.*occurrences insource_hsolverproduction code: 60 → 5. What remains isDiagoIterAssist'sbasis_type/calculationswitches, planned as a separate PR, plus one comment insimple_pexsi.cpp.Testing
In the two earlier PRs I had to flag large unverified areas. That gap is much smaller here: by writing minimal stand-in headers for the third-party APIs that are unavailable locally, all six diagonalizers pass
g++ -fsyntax-only, each in the configuration it is actually built in:diago_lapack.cpp__MPIdiago_scalapack.cpp__MPIwith the real header; also force-included thepdsygvx_/pzhegvx_prototypes copied verbatim fromscalapack_connector.hto fully type-check the&this->nlocalsitesdiago_elpa.cpp__MPI+ ELPA stand-indiago_elpa_native.cpp__MPI+ ELPA stand-in withELPA_WITH_NVIDIA_GPU_VERSION=1, so the newuse_gpubranch is genuinely compileddiago_cusolver.cpp__CUDA+ CUDA/cuBLAS/cuSOLVER stand-insdiago_cusolvermp.cpp__CUSOLVERMP+ cusolverMp/NCCL stand-insdiago_cusolvermp.cppis worth calling out: no CI job builds__CUSOLVERMP, so this local check is the only compile coverage it gets.hsolver_lcao.cppwas checked across six flag combinations (bare,__MPI,+__ELPA,+__CUDA,+__CUSOLVERMP, and all of them together with__PEXSI), covering every branch of the eleven construction sites. As a side effect this also closes theparakSolvegap I flagged in #7711, which sits inside#ifdef __MPI.esolver_ks_lcao.cpp,esolver_ks_lcao_tddft.cpp,LCAO_set.cppandcal_mw_from_lambda.cppalso pass.Not covered locally: the three test files need gtest, which is unavailable here. Their changes are limited to dropping
PARAMwrites, adding twointconstructor arguments, and deleting the dead include blocks;MODULE_HSOLVER_LCAObuilds in the default CI matrix.Pre-existing issue noticed, not addressed
Compiling
diago_elpa.cppwithout__MPIproduces four errors:elpa_num_threadandlastmpinumare declared inside#ifdef __MPIin the header, but their static definitions at the bottom are unguarded. I verified against the unmodified file (git stash) that the error set is identical before and after this change.DiagoElparequires MPI anyway, so no supported build configuration is affected, and I left it alone rather than widen this PR — happy to fix it separately if wanted.Behaviour is intended to be unchanged throughout: every injected value is the same
PARAMfield that was previously read directly, sourced at the esolver layer.🤖 Generated with Claude Code