Add spin-summed RKS density setting#208
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an explicit opt-in integrator setting to interpret RKS density matrices as spin-summed closed-shell densities (instead of GauXC’s default one-spin convention), and threads that setting through host/device reference paths and shell-batched wrappers. It also adds a standalone-driver keyword for reproducer workflows and a focused regression check.
Changes:
- Add
IntegratorSettingsKS::rks_density_matrix_is_spin_summed(defaultfalse) to control the RKSxmat_facscaling. - Apply the setting to host/device replicated integrator
xmat_faccomputation and propagate through shell-batched wrapper call chains. - Add a standalone-driver keyword and a regression test exercising spin-summed RKS density input on an LDA reference path.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
include/gauxc/xc_integrator_settings.hpp |
Adds the new rks_density_matrix_is_spin_summed flag to KS settings. |
src/xc_integrator/replicated/host/reference_replicated_xc_host_integrator_exc_vxc.hpp |
Uses the new flag to choose the RKS xmat_fac (2 vs 1) in EXC/VXC host reference path. |
src/xc_integrator/replicated/host/reference_replicated_xc_host_integrator_exc_grad.hpp |
Propagates the flag into EXC gradient host reference xmat_fac selection. |
src/xc_integrator/replicated/host/reference_replicated_xc_host_integrator_fxc_contraction.hpp |
Uses the new flag for xmat_fac in host FXC contraction path. |
src/xc_integrator/replicated/device/incore_replicated_xc_device_integrator.hpp |
Extends device local-work APIs to accept settings for downstream scaling decisions. |
src/xc_integrator/replicated/device/incore_replicated_xc_device_integrator_exc_vxc.hpp |
Threads settings into device EXC/VXC local work and applies spin-summed handling in xmat_fac. |
src/xc_integrator/replicated/device/incore_replicated_xc_device_integrator_exc_grad.hpp |
Propagates the flag into device EXC gradient xmat_fac selection. |
src/xc_integrator/replicated/device/incore_replicated_xc_device_integrator_fxc_contraction.hpp |
Threads settings into device FXC contraction local work and applies spin-summed handling in xmat_fac. |
src/xc_integrator/replicated/device/incore_replicated_xc_device_integrator_exc.hpp |
Updates device EXC-only path to pass settings through (currently has a correctness issue noted in comments). |
src/xc_integrator/shell_batched/shell_batched_replicated_xc_integrator.hpp |
Propagates settings through shell-batched replicated integrator interfaces. |
src/xc_integrator/shell_batched/shell_batched_replicated_xc_integrator_exc.hpp |
Passes settings down into shell-batched EXC local-work path. |
src/xc_integrator/shell_batched/shell_batched_replicated_xc_integrator_exc_vxc.hpp |
Passes settings down into shell-batched EXC/VXC task execution path. |
src/xc_integrator/shell_batched/shell_batched_replicated_xc_integrator_exc_grad.hpp |
Updates NYI shell-batched EXC-grad stubs to accept/unused settings. |
src/xc_integrator/shell_batched/shell_batched_replicated_xc_integrator_fxc_contraction.hpp |
Updates NYI shell-batched FXC contraction stubs to accept/unused settings. |
tests/standalone_driver.cxx |
Adds input keyword and forwards it to selected integrator calls (gap noted in comments). |
tests/xc_integrator.cxx |
Adds a regression check for spin-summed RKS density input (LDA-only) (coverage gap noted in comments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Passing nullptr for VXCs disables VXC entirely | ||
| nullptr, 0, nullptr, 0, nullptr, 0, nullptr, 0, EXC, &N_EL, | ||
| tasks.begin(), tasks.end(), *device_data_ptr); | ||
| tasks.begin(), tasks.end(), *device_data_ptr, ks_settings); |
There was a problem hiding this comment.
Fixed in 8646de6 by passing the existing settings object through the device EXC-only path. I also checked a CUDA build path; the previous undefined-identifier failure is gone.
| OPTIONAL_KEYWORD( "GAUXC.INTEGRATE_VXC", integrate_vxc, bool ); | ||
| OPTIONAL_KEYWORD( "GAUXC.INTEGRATE_EXX", integrate_exx, bool ); | ||
| OPTIONAL_KEYWORD( "GAUXC.INTEGRATE_EXC_GRAD", integrate_exc_grad, bool ); | ||
| OPTIONAL_KEYWORD( "GAUXC.RKS_DENSITY_MATRIX_IS_SPIN_SUMMED", rks_density_matrix_is_spin_summed, bool ); |
There was a problem hiding this comment.
Fixed in 8646de6. The standalone driver now constructs one RKS KS settings object from the parsed keyword and reuses it for the RKS eval_exc_vxc, eval_exc_grad, and eval_fxc_contraction paths.
| CHECK(EXC3 == Approx(EXC_ref)); | ||
| auto [ EXC4, VXC4 ] = integrator.eval_exc_vxc( P_spin_summed, spin_summed_settings ); | ||
| CHECK(EXC4 == Approx(EXC_ref)); | ||
| (void)VXC4; |
There was a problem hiding this comment.
Fixed in 8646de6. The spin-summed RKS regression now compares VXC4 against VXC_ref with the same norm-per-basis-function style tolerance used by the existing VXC checks.
|
Thanks for the patch, but help me try to understand: we want to add all of this plumbing because we expect I'd agree that there's a (historical) inconsistency in the RKS + UKS/GKS paths where the latter already works in the Pauli basis: ... etc. But I'd argue the solution is to simply make the conventions consistent rather than adding a flag that needs to track the convention for bookkeeping. If there's interest in that, we can absolutely look into that once I get some feedback from stakeholders. As for this - Because it's a trivial rescaling that can happen locally in the calling program, I'm inclined to say this is more work than it's worth. Open to counterarguments. |
|
Dear David, Yes, that is the convention mismatch this PR tries to make explicit. For context, this PR was split out from #207 after Sebastian asked to separate the core GauXC convention/API question from the Skala/Fortran-facing changes. #207 needs this only as plumbing for CP2K/Skala, but the actual question of whether GauXC should expose such a setting clearly belongs in the core library, which is why I opened #208 against I agree that the factor-of-two conversion is mathematically trivial. The reason I did not keep it only in the caller is that the convention then becomes implicit and has to be remembered consistently across EXC, VXC, EXC gradients, FXC contractions, and standalone HDF5 reproducer workflows. In the CP2K path the natural RKS object is the closed-shell spin-summed AO density matrix, while the current GauXC RKS path expects the one-spin matrix. An explicit opt-in at the KS integrator boundary made that convention visible at the point where GauXC forms the real-space density, while preserving the current default behavior. That said, I also agree that a broader convention cleanup making RKS/UKS/GKS consistent would be the cleaner long-term solution. I am not trying to force this particular flag if GauXC prefers to handle the mismatch in the caller for now or to move directly toward a unified convention. So I am happy to follow your preference here: keep this as a narrow temporary opt-in if useful for external interfaces, or close #208 and keep the rescaling downstream until GauXC settles the broader convention question. Greetings, |
This separates the core GauXC part of the spin-summed RKS density-matrix handling from the Skala/Fortran-facing API exposure.
GauXC keeps the existing default RKS convention: an RKS density matrix is interpreted as a one-spin density and the closed-shell density is formed internally with the usual factor of two. Some external interfaces provide the already spin-summed closed-shell density matrix. This PR adds an explicit opt-in setting for that case without changing the default behavior.
Changes:
IntegratorSettingsKS::rks_density_matrix_is_spin_summedxmat_facpathsNo Skala, OneDFT, C API, or Fortran API exposure is included here; those belong in a follow-up change on the
skalabranch once this core convention is agreed.Validation performed on Spark:
masterstandalone_driverandgauxc_testgauxc_test "XC Integrator" -c "Benzene / SVWN5 / cc-pVDZ" -c Host -c Reference --successsuccessfullyN_EL = 9.999998605222EXC = -9.2473984901214.093047e-13 6.159873e-14 -4.648139e-01-1.646459e-13 3.214351e-01 2.324069e-01-2.447600e-13 -3.214351e-01 2.324069e-01The full serial GauXC test target was also tried, but existing MGGA reference sections fail in this environment independently of this focused RKS convention check.