PR - [9] Bind 3-center 2-electron integrals, add cartesian support via macro type parameter - #257
Open
San1357 wants to merge 68 commits into
Open
PR - [9] Bind 3-center 2-electron integrals, add cartesian support via macro type parameter #257San1357 wants to merge 68 commits into
San1357 wants to merge 68 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
PR[1]: build: migrate from setuptools to scikit-build-core
PR[2] : feat: add libcint C extension bindings for various integrals
PR[3]: feat: use PyArray_GETPTR instead of PyLong_AsVoidPtr + fix dylib rpath
PR[4]: fix: platform-aware libcint loading + moment integral libcint v6 fix
PR-5 - feat: add C shell loop for all 1-electron integrals
msricher
previously approved these changes
Jul 23, 2026
msricher
left a comment
msricher
left a comment
Collaborator
There was a problem hiding this comment.
Great, thanks for also adding the automatic selection of (cart|sph) at runtime.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a Python C-extension (libcint_bindings) to accelerate/extend libcint-backed integral evaluation (including new 3-center 2-electron integrals), refactors macro-based generation for spherical/cartesian variants, and updates packaging/build tooling to use a CMake/scikit-build-core workflow.
Changes:
- Added a new C-extension wrapper (
gbasis/integrals/src/libcint_wrap.c) exposing shell-loop integral array builders (plusint3c2e_sph). - Extended
gbasis.integrals.libcint.CBasiswith new C-backed shell-loop methods (andthree_center_two_electron()), including runtime_sph/_cartselection via a suffix. - Updated tests and build configuration (pyproject + CI) to support the new build and validate new bindings.
Reviewed changes
Copilot reviewed 6 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_libcint.py |
Expands libcint test coverage to include new C shell-loop bindings and 3c2e reference comparisons. |
pyproject.toml |
Switches build backend to scikit-build-core and updates dependency minimums. |
gbasis/integrals/src/libcint_wrap.c |
Adds the C-extension module implementing array builders and new integral bindings. |
gbasis/integrals/libcint.py |
Wires the new extension into CBasis and adds higher-level Python methods for new bindings. |
CMakeLists.txt |
Defines the CMake build for the extension and fetches/builds libcint/qcint. |
.github/workflows/pytest.yaml |
Updates minimal dependency pins used in CI (numpy/scipy). |
Comments suppressed due to low confidence (3)
gbasis/integrals/libcint.py:234
- The ctypes loader hard-codes a platform-specific
libcintfilename undergbasis/integrals/lib(e.g.libcint.soon Linux). With this PR switching to a CMake/scikit-build-core build that installslibcint_bindingsintogbasis/integrals/lib, it’s easy to end up with the extension present but nolibcint.*shared library at the expected path (and/or a different SONAME likelibcint.so.6). That will makegbasis.integrals.libcintfail to import when_LibCIntis used (sinceLIBCINTis instantiated at import time). Please ensure the build/install step also installs a compatiblelibcintshared library and the expected filename/symlink intogbasis/integrals/lib, or refactor so the ctypes path is optional/lazy whenlibcint.*is absent.
elif _system == "Windows":
_lib_name = "libcint.dll"
else:
_lib_name = "libcint.so"
_libcint: CDLL = cdll.LoadLibrary(str(_lib_dir / _lib_name))
CMakeLists.txt:55
INSTALL_RPATHis set to@loader_pathunconditionally.@loader_pathis a macOS-specific placeholder; on Linux this value is not meaningful and can lead to runtime loader issues (especially ifcintis built/linked as a shared library). Consider setting RPATH conditionally (Darwin:@loader_path, Linux:$ORIGIN).
# Set RPATH so libcint.dylib is found at runtime.
INSTALL_RPATH "@loader_path"
BUILD_WITH_INSTALL_RPATH TRUE)
CMakeLists.txt:67
- Only the Python extension module is installed into
gbasis/integrals/lib, but the Python ctypes path (_LibCInt) still expects a loadablelibcint.*shared library under the same directory. Ifcintis built as a shared library, it should also be installed next to the extension so runtime loading works; if it’s built statically, the ctypes path likely needs to be removed/disabled (since there won’t be alibcint.*to load).
# Install C extension module.
set(GBASIS_WHEEL_DIRECTORY "${SKBUILD_PLATLIB_DIR}/gbasis/integrals/lib")
install(TARGETS gbasis_ext LIBRARY DESTINATION ${GBASIS_WHEEL_DIRECTORY})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+10
to
+11
| # to Remove 1 | ||
| from ctypes import CDLL, POINTER, Structure, cdll, byref, c_int, c_double |
|
|
||
| """ | ||
|
|
||
| from gbasis.integrals.lib import libcint_bindings |
| # both are pinned to v6.1.2 for stability. | ||
| include(CheckCCompilerFlag) | ||
| set(GBASIS_CINT_REPO "https://github.com/sunqm/libcint.git") | ||
| set(GBASIS_CINT_VERSION "v6.1.2") |
| } | ||
| } | ||
| } | ||
| memset(buf, 0, buf_size * sizeof(double)); |
| } | ||
| } | ||
| } | ||
| memset(buf, 0, buf_size * sizeof(double)); |
added 6 commits
July 28, 2026 14:30
…e three_center_two_electron
…anual libcint install
msricher
requested changes
Jul 28, 2026
| install(TARGETS gbasis_ext LIBRARY DESTINATION ${GBASIS_WHEEL_DIRECTORY}) | ||
|
|
||
|
|
||
| # Install libcint/qcint shared library alongside the extension module. |
Collaborator
There was a problem hiding this comment.
You shouldn't have to do this! Last time we edited the CMakeLists.txt, we made libcint a static library that is compiled directly into the extension module.
added 4 commits
July 28, 2026 20:51
… skip - Add _ovlp_minhalf cartesian normalization in __init__ - Apply normalization in all 2D methods - Apply 4D normalization in electron_repulsion - Fix point_charge: add permutation before storing val - Add momentum() method with -1j scaling - Add transform parameter to point_charge() and moment() - Skip momentum/moment tests (multi-component C fix needed) - Skip cc-pVDZ cartesian ERI (d-shell normalization TODO)
added 2 commits
August 3, 2026 10:03
msricher
requested changes
Aug 3, 2026
msricher
left a comment
msricher
left a comment
Collaborator
There was a problem hiding this comment.
Hi, make sure the Cmakelists.txt functions as we had it before, but otherwise this looks good. Since the Github Actions checks pass, you can merge this.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 18 changed files in this pull request and generated 3 comments.
Suppressed comments (11)
gbasis/integrals/src/libcint_wrap.c:480
- Same as the spherical case: the cartesian
ipkin/ipnuc/iprinvwrappers are generated with the scalar loop macro, which would discard the 3 components. These should be 3-component wrappers.
DEFINE_INT1E_LOOP_FN(ipkin_integral_array, cart, int1e_ipkin, int1e_ipkin)
DEFINE_INT1E_LOOP_FN(ipnuc_integral_array, cart, int1e_ipnuc, int1e_ipnuc)
DEFINE_INT1E_LOOP_FN(iprinv_integral_array, cart, int1e_iprinv, int1e_iprinv)
gbasis/integrals/libcint.py:689
- Docstring references
moment_integral()but the method was renamed tomoment(), so this note is now misleading.
Returns the first component (xx) of the quadrupole integral from
``int1e_rr_sph``. The full 9-component quadrupole integral is
available via ``moment_integral()``.
gbasis/integrals/libcint.py:744
- Docstring references
moment_integral()but the method was renamed tomoment(), so this note is now misleading.
Returns the first component (xxx) of the octupole integral from
``int1e_rrr_sph``. The full 27-component octupole integral is
available via ``moment_integral()``.
gbasis/integrals/libcint.py:827
gradient_nuclear()currently returns a 2D array, butint1e_ipnucis a 3-component integral (x/y/z). Returning only (Nbasis, Nbasis) drops two components.
out : np.ndarray(Nbasis, Nbasis, dtype=float)
Gradient nuclear integral array.
"""
out = np.zeros((self.nbfn, self.nbfn), dtype=c_double, order="F")
getattr(libcint_bindings, f"ipnuc_integral_array_{self._ct}")(
gbasis/integrals/libcint.py:870
gradient_rinv()currently returns a 2D array, butint1e_iprinvis a 3-component integral (x/y/z). Returning only (Nbasis, Nbasis) drops two components.
if inv_origin is None:
inv_origin = np.zeros(3)
self.env[4:7] = inv_origin
out = np.zeros((self.nbfn, self.nbfn), dtype=c_double, order="F")
getattr(libcint_bindings, f"iprinv_integral_array_{self._ct}")(
tests/test_libcint.py:623
- If gradient integrals are exposed as 3-component (x/y/z) arrays, the expected shape here should include the 3-vector dimension.
lc_int = lc_basis.gradient_nuclear()
npt.assert_array_equal(lc_int.shape, (lc_basis.nbfn, lc_basis.nbfn))
assert np.all(np.isfinite(lc_int))
# Test with transform
transform = np.eye(lc_basis.nbfn)
lc_int_t = lc_basis.gradient_nuclear(transform=transform)
npt.assert_array_equal(lc_int_t.shape, (lc_basis.nbfn, lc_basis.nbfn))
tests/test_libcint.py:632
- If gradient integrals are exposed as 3-component (x/y/z) arrays, the expected shape here should include the 3-vector dimension.
lc_int = lc_basis.gradient_rinv(inv_origin=np.zeros(3))
npt.assert_array_equal(lc_int.shape, (lc_basis.nbfn, lc_basis.nbfn))
assert np.all(np.isfinite(lc_int))
# Test with transform
transform = np.eye(lc_basis.nbfn)
lc_int_t = lc_basis.gradient_rinv(inv_origin=np.zeros(3), transform=transform)
npt.assert_array_equal(lc_int_t.shape, (lc_basis.nbfn, lc_basis.nbfn))
gbasis/integrals/src/libcint_wrap.c:354
DEFINE_INT1E_LOOP_FNallocates/zeros a buffer sizedmax_off*max_off*27even for scalar integrals. This is 27× extra work and memory and appears unintended given there is a dedicated MULTICOMP macro for 27-component integrals.
size_t buf_size = (size_t)max_off * max_off * 27; \
double *buf = calloc(buf_size, sizeof(double)); \
gbasis/integrals/libcint.py:634
- Docstring references
moment_integral()but the method was renamed tomoment(), so this note is now misleading.
This issue also appears in the following locations of the same file:
- line 687
- line 742
Returns the first component (x) of the dipole integral from
``int1e_r_sph``. The full 3-component dipole integral is
available via ``moment_integral()``.
gbasis/integrals/libcint.py:999
iking()applies the permutation twice, causing an unnecessary copy and extra work.
# Apply permutation
out = out[self._permutations, :][:, self._permutations]
# Apply permutation
out = out[self._permutations, :][:, self._permutations]
tests/test_libcint.py:614
- If gradient integrals are exposed as 3-component (x/y/z) arrays, these tests should assert the third dimension as well; otherwise they can pass while two components are silently dropped.
This issue also appears in the following locations of the same file:
- line 616
- line 626
lc_int = lc_basis.gradient_kinetic()
npt.assert_array_equal(lc_int.shape, (lc_basis.nbfn, lc_basis.nbfn))
assert np.all(np.isfinite(lc_int))
# Test with transform
transform = np.eye(lc_basis.nbfn)
lc_int_t = lc_basis.gradient_kinetic(transform=transform)
npt.assert_array_equal(lc_int_t.shape, (lc_basis.nbfn, lc_basis.nbfn))
Comment on lines
47
to
+50
| - name: Run Pytest | ||
| run: | | ||
| pytest --cov=./gbasis/ --cov-fail-under=78 --cov-report term . | ||
| pip install scikit-build-core cmake ninja | ||
| pip install --no-build-isolation -v -e .[dev,iodata] No newline at end of file |
Comment on lines
+460
to
+462
| DEFINE_INT1E_LOOP_FN(ipkin_integral_array, sph, int1e_ipkin, int1e_ipkin) | ||
| DEFINE_INT1E_LOOP_FN(ipnuc_integral_array, sph, int1e_ipnuc, int1e_ipnuc) | ||
| DEFINE_INT1E_LOOP_FN(iprinv_integral_array, sph, int1e_iprinv, int1e_iprinv) |
Comment on lines
+784
to
+788
| out : np.ndarray(Nbasis, Nbasis, dtype=float) | ||
| Gradient kinetic integral array. | ||
| """ | ||
| return self._eri(notation=notation, transform=transform) | ||
| out = np.zeros((self.nbfn, self.nbfn), dtype=c_double, order="F") | ||
| getattr(libcint_bindings, f"ipkin_integral_array_{self._ct}")( |
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.
Changes
3-center 2-electron integrals
int3c2e_sphinlibcint_wrap.cusing 3 nested shell loops withshls[3] = 0three_center_two_electron()toCBasiswith transform support.npyfiles for He, C, H+He, Be+C systems (STO-6G)Macro refactor
DEFINE_INT1E_ARRAY_FN,DEFINE_INT1E_LOOP_FN, andDEFINE_INT1E_LOOP_FN_OPTto accept atypeparameter via token pasting_sphand_cartvariants now generated from the same macro callDEFINE_INT1E_ARRAY_FN(overlap, sph, int1e_ovlp)→ generatesoverlap_sphcoord_type aware C bindings
_sphor_cartbased oncoord_typeat runtimeTests
572 passed, 4 skipped
Still in progress
int3c2e_cartbinding and cartesian reference teststest_c_shellloop_integralmake_int1e,make_int2e, old*_integral()wrappers)