Conversation
dd396c2 to
5881a76
Compare
1d61b38 to
83841ad
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds a comprehensive C API for the GauXC library, enabling integration with C codebases and other languages that interface with C. The implementation wraps core GauXC C++ functionality including molecular structures, basis sets, integration grids, load balancing, and XC integrators.
Changes:
- Added C API bindings for core classes (Molecule, Atom, BasisSet, Shell, etc.)
- Implemented factory patterns for runtime environments, load balancers, molecular weights, and XC integrators
- Created C/C++ compatible enum definitions with mappings between C and C++ versions
- Added HDF5 I/O support for C API
- Refactored configuration headers to separate C and C++ concerns
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| include/gauxc/*.h | New C API header files defining opaque handles and functions for core types |
| include/gauxc/util/c_*.hpp | Helper headers for casting between C handles and C++ objects |
| src/c_*.cxx | C API implementation files wrapping C++ functionality |
| include/gauxc/enum.h | C-compatible enum definitions |
| include/gauxc/enums.hpp | C++ enum class definitions mapped to C enums |
| include/gauxc/gauxc_config.h.in | C-compatible configuration header |
| include/gauxc/gauxc_config.hpp | C++ configuration header including C header |
| tests/moltypes_test.cxx | Basic C API interoperability test |
| src/CMakeLists.txt | CMake integration for conditional C API compilation |
| src/external/c_hdf5_*.cxx | HDF5 I/O functions for C API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- provide C enums - add atom and molecule types - add basis set and shell definitions - add molecule grid and runtime environment - add load balancer to C API - add molecular weights for C API - add functional class wrapping ExchCXX - add xc integrator and matrix type - add references for functionals - add support for reading and writing from HDF5 in C
| @@ -0,0 +1,112 @@ | |||
| /** | |||
There was a problem hiding this comment.
Global comment: I'd prefer that C-related things (and similarly for #174) are placed in gauxc/c_api and gauxc/fortran_api respectively. The top-level is messy enough as it is.
| @@ -0,0 +1,47 @@ | |||
| /** | |||
There was a problem hiding this comment.
Similarly, this seems to be glue to stitch together the C/C++ APIs? I'd combine all of these types of utilities into either a single file, or into a single directory.
include/gauxc/util/c_basisset.hpp
Outdated
| static inline BasisSet<double>* get_basisset_ptr(C::GauXCBasisSet basis) noexcept { | ||
| return static_cast<BasisSet<double>*>(basis.ptr); | ||
| } | ||
| static inline Shell<double> convert_shell(C::GauXCShell shell, bool normalize) noexcept { |
There was a problem hiding this comment.
Why is this not namespaced? This can't be called from C.
include/gauxc/util/c_basisset.hpp
Outdated
| static inline BasisSet<double>* get_basisset_ptr(C::GauXCBasisSet basis) noexcept { | ||
| return static_cast<BasisSet<double>*>(basis.ptr); | ||
| } | ||
| static inline Shell<double> convert_shell(C::GauXCShell shell, bool normalize) noexcept { |
There was a problem hiding this comment.
Unless the functions are very small, I'd prefer that they get compiled into the library rather than living in a header.
|
|
||
| namespace GauXC::detail { | ||
|
|
||
| static inline char* strdup(const char* str) { |
There was a problem hiding this comment.
These kinds of functions make me nervous - is there a reason we don't just use the POSIX/C23 version of this? Also, why is this in the status header?
| #pragma once | ||
| #include <gauxc/gauxc_config.h> | ||
|
|
||
| #ifdef GAUXC_HAS_MPI |
There was a problem hiding this comment.
I'd prefer that we do not duplicate this kind of code.
| */ | ||
| #pragma once | ||
|
|
||
| #ifdef __cplusplus |
There was a problem hiding this comment.
This kind of include scheme makes me very nervous - are the functions in these headers guaranteed to have the same ABI?
| #endif | ||
|
|
||
| enum GauXC_Functional { | ||
| /// @brief Slater exchange & Vosko, Wilk & Nusair correlation (VWN3) |
There was a problem hiding this comment.
I think we talked about this in this PR wavefunction91/ExchCXX#49, and I'd prefer that we remain consistent.
| @@ -31,10 +32,4 @@ | |||
| #ifdef GAUXC_HAS_DEVICE | |||
| #cmakedefine GAUXC_GPU_XC_MAX_AM @GAUXC_GPU_XC_MAX_AM@ | |||
| #cmakedefine GAUXC_GPU_SNLINK_MAX_AM @GAUXC_GPU_SNLINK_MAX_AM@ | |||
| #endif | |||
|
|
|||
| #if defined(__CUDACC__) || defined(__HIPCC__) | |||
There was a problem hiding this comment.
Why can't this just stay here?
| @@ -0,0 +1,162 @@ | |||
| /** | |||
There was a problem hiding this comment.
Global comment on use of managed matrix types in C - not a fan. I'd much prefer that we delegate all memory management of matrices to the calling program. In fact, we already do that under the hood. When I had considered writing a C-API in the past, my intention was always to expose the raw pointer API to the user and let them handle it (in a BLAS/LAPACK style convention). Do you have thoughts on this @susilehtola - I just remember the C-API being a point you had brought up several times.
There was a problem hiding this comment.
I agree, the C API should use external memory
There was a problem hiding this comment.
The current API of the XC integrator is allocating memory for return values like vxc. We would either need to extend the API to allow passing matrices as output types or have the possibility to pass an allocator callback to the XC integrator.
I can make a separate PR to implement a new API for the XC integrator to not allocate matrices itself. Any preference?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void gauxc_integrator_eval_exc_vxc_rks( | ||
| GauXCStatus* status, | ||
| const GauXCIntegrator integrator, | ||
| const GauXCMatrix density_matrix, | ||
| double* exc_out, | ||
| GauXCMatrix* vxc_matrix | ||
| ) { | ||
| vxc_matrix->ptr = nullptr; | ||
|
|
||
| try { | ||
| auto& dm = *detail::get_matrix_ptr(density_matrix); | ||
|
|
||
| auto [exc, vxc] = integrator.owned | ||
| ? detail::get_xc_integrator_ptr(integrator)->eval_exc_vxc(dm) | ||
| : detail::get_xc_integrator_shared(integrator)->get()->eval_exc_vxc(dm); | ||
|
|
||
| *exc_out = exc; | ||
| vxc_matrix->ptr = new detail::CMatrix(std::move(vxc)); | ||
| status->code = 0; |
There was a problem hiding this comment.
The output GauXCMatrix* vxc_matrix handle is only partially populated: hdr.type is never set, and any pre-existing vxc_matrix->ptr is overwritten without being freed (memory leak). Initialize vxc_matrix->hdr to GauXC_Type_Matrix and either require/verify vxc_matrix is empty or delete/reuse any existing matrix storage before assigning a new pointer.
| void gauxc_integrator_eval_exc_vxc_gks( | ||
| GauXCStatus* status, | ||
| const GauXCIntegrator integrator, | ||
| const GauXCMatrix density_matrix_s, | ||
| const GauXCMatrix density_matrix_z, | ||
| const GauXCMatrix density_matrix_x, | ||
| const GauXCMatrix density_matrix_y, | ||
| double* exc_out, | ||
| GauXCMatrix* vxc_matrix_s, | ||
| GauXCMatrix* vxc_matrix_z, | ||
| GauXCMatrix* vxc_matrix_x, | ||
| GauXCMatrix* vxc_matrix_y | ||
| ) { | ||
| vxc_matrix_s->ptr = nullptr; | ||
| vxc_matrix_z->ptr = nullptr; | ||
| vxc_matrix_x->ptr = nullptr; | ||
| vxc_matrix_y->ptr = nullptr; | ||
|
|
||
| try { | ||
| auto& dm_s = *detail::get_matrix_ptr(density_matrix_s); | ||
| auto& dm_z = *detail::get_matrix_ptr(density_matrix_z); | ||
| auto& dm_x = *detail::get_matrix_ptr(density_matrix_x); | ||
| auto& dm_y = *detail::get_matrix_ptr(density_matrix_y); | ||
|
|
||
| auto [exc, vxc_s, vxc_z, vxc_x, vxc_y] = integrator.owned | ||
| ? detail::get_xc_integrator_ptr(integrator)->eval_exc_vxc(dm_s, dm_z, dm_x, dm_y) | ||
| : detail::get_xc_integrator_shared(integrator)->get()->eval_exc_vxc(dm_s, dm_z, dm_x, dm_y); | ||
|
|
||
| *exc_out = exc; | ||
| vxc_matrix_s->ptr = new detail::CMatrix(std::move(vxc_s)); | ||
| vxc_matrix_z->ptr = new detail::CMatrix(std::move(vxc_z)); | ||
| vxc_matrix_x->ptr = new detail::CMatrix(std::move(vxc_x)); | ||
| vxc_matrix_y->ptr = new detail::CMatrix(std::move(vxc_y)); | ||
| status->code = 0; |
There was a problem hiding this comment.
Same as other eval_exc_vxc_* APIs: all vxc_matrix_* outputs should have hdr.type set and should not leak by overwriting an existing ptr. Consider either (1) filling preallocated matrices (resize + write) or (2) explicitly deleting any existing ptr before assigning a new allocation and documenting ownership clearly.
| typedef struct GauXCStatus { | ||
| int code; | ||
| char* message; | ||
| } GauXCStatus; |
There was a problem hiding this comment.
GauXCStatus has a raw char* message, but there’s no documented/implemented initialization/reset contract. Because most C API entry points only set status->code (not status->message), C++ callers that default-initialize GauXCStatus can end up with an indeterminate message pointer and crash on gauxc_status_delete. Consider (a) documenting that callers must zero-initialize ({0,nullptr}), and (b) having every API entry point clear/free any previous message and set message=nullptr on success.
| C::GauXCStatus status; | ||
| C::GauXCMolecule mol = C::gauxc_molecule_new(&status); | ||
| CHECK(status.code == 0); |
There was a problem hiding this comment.
GauXCStatus is a POD and C::GauXCStatus status; leaves status.message uninitialized in C++. Since the C API doesn’t consistently null out/clear status.message on success, later calls like gauxc_status_delete(&status) could delete[] a garbage pointer. Prefer zero-initializing here (e.g., {} / {0,nullptr}) and/or ensure the C API resets status.message on entry/success.
| GauXCMatrix gauxc_matrix_empty( | ||
| GauXCStatus* status | ||
| ) { | ||
| status->code = 0; | ||
| GauXCMatrix matrix; | ||
| matrix.hdr = GauXCHeader{GauXC_Type_Matrix}; | ||
| matrix.ptr = new detail::CMatrix(); | ||
|
|
There was a problem hiding this comment.
gauxc_matrix_empty performs new detail::CMatrix() without a try/catch. If allocation throws (e.g., std::bad_alloc), the exception will cross the C API boundary. Wrap the allocation like gauxc_matrix_new does and set status->code/message + return {.ptr=nullptr} on failure.
| auto polar = polarized ? ExchCXX::Spin::Polarized : ExchCXX::Spin::Unpolarized; | ||
| functional_type func; | ||
| if(ExchCXX::functional_map.key_exists(functional_spec)) { | ||
| func = functional_type( ExchCXX::Backend::builtin, ExchCXX::functional_map.value(functional_spec), | ||
| polar ); | ||
| } | ||
| #ifdef EXCHCXX_ENABLE_LIBXC | ||
| else { | ||
| std::vector<std::pair<double, ExchCXX::XCKernel>> funcs; | ||
| std::vector<std::string> libxc_names; | ||
| detail::split(libxc_names, functional_spec, ","); | ||
| for( auto n : libxc_names ) { | ||
| funcs.push_back( {1.0, ExchCXX::XCKernel(ExchCXX::libxc_name_string(n), polar)} ); | ||
| } | ||
| func = functional_type(funcs); | ||
| } | ||
| #endif | ||
|
|
||
| functional.ptr = new functional_type( std::move(func) ); | ||
| } catch (std::exception& e) { |
There was a problem hiding this comment.
If functional_spec is not in ExchCXX::functional_map and EXCHCXX_ENABLE_LIBXC is not enabled, func is left without being assigned from the input spec, yet the code still constructs and returns a functional_type from it. This should instead report an error (set nonzero status / throw) when the functional string is unknown and libxc support isn’t available.
| void gauxc_integrator_eval_exc_vxc_uks( | ||
| GauXCStatus* status, | ||
| const GauXCIntegrator integrator, | ||
| const GauXCMatrix density_matrix_s, | ||
| const GauXCMatrix density_matrix_z, | ||
| double* exc_out, | ||
| GauXCMatrix* vxc_matrix_s, | ||
| GauXCMatrix* vxc_matrix_z | ||
| ) { | ||
| vxc_matrix_s->ptr = nullptr; | ||
| vxc_matrix_z->ptr = nullptr; | ||
|
|
||
| try { | ||
| auto& dm_s = *detail::get_matrix_ptr(density_matrix_s); | ||
| auto& dm_z = *detail::get_matrix_ptr(density_matrix_z); | ||
|
|
||
| auto [exc, vxc_s, vxc_z] = integrator.owned | ||
| ? detail::get_xc_integrator_ptr(integrator)->eval_exc_vxc(dm_s, dm_z) | ||
| : detail::get_xc_integrator_shared(integrator)->get()->eval_exc_vxc(dm_s, dm_z); | ||
|
|
||
| *exc_out = exc; | ||
| vxc_matrix_s->ptr = new detail::CMatrix(std::move(vxc_s)); | ||
| vxc_matrix_z->ptr = new detail::CMatrix(std::move(vxc_z)); | ||
| status->code = 0; |
There was a problem hiding this comment.
Same as RKS: vxc_matrix_s/vxc_matrix_z output handles don’t have hdr.type initialized and their ptr fields are overwritten without freeing any existing allocation. This can break gauxc_object_delete (type dispatch) and can leak memory if the caller reuses output matrices.
| #ifdef GAUXC_HAS_C | ||
| SECTION("C HDF5 IO") { | ||
| C::GauXCStatus status; |
There was a problem hiding this comment.
The "C HDF5 IO" section is only guarded by GAUXC_HAS_C, but the declarations in <gauxc/external/hdf5.h> are only available when GAUXC_HAS_HDF5 is set. If C API is enabled but HDF5 is disabled, this test will fail to compile. Guard this section (and/or the include) with GAUXC_HAS_HDF5 as well.
| C::GauXCStatus status; | ||
|
|
There was a problem hiding this comment.
Same issue as earlier: C::GauXCStatus status; does not initialize status.message in C++. Use value-initialization ({}) to avoid leaving message as an indeterminate pointer (which can later crash on gauxc_status_delete).
| void gauxc_object_delete(GauXCStatus* status, void** obj) { | ||
| status->code = 0; | ||
| if(obj == nullptr) return; | ||
|
|
||
| struct GauXCObject { | ||
| GauXCHeader hdr; | ||
| }; | ||
|
|
||
| GauXCHeader* header = &reinterpret_cast<struct GauXCObject*>(*obj)->hdr; | ||
|
|
There was a problem hiding this comment.
gauxc_object_delete dereferences *obj unconditionally (reinterpret_cast<...>(*obj)), but only checks obj == nullptr. If the caller passes a null object pointer (common when deleting optional handles), this will crash. Add a *obj == nullptr guard before reading the header, and consider nulling *obj after a successful delete since the API takes a void**.
GauXC::Moleculeingauxc/molecule.hppGauXC::Atomingauxc/atom.hppGauXC::BasisSetingauxc/basisset.hpp(double only)GauXC::Shellingauxc/shell.hppset_shell_tolerancemethodGauXC::RuntimeEnvironmentandGauXC::DeviceRuntimeEnvironmentset_buffer,comm_rank,comm_sizeGauXC::AtomicGridSizeDefault,GauXC::PruningScheme,GauXC::RadialQuadingauxc/enum.hppGauXC::MolGridFactorycreate_default_molgridGauXC::MolGridGauXC::ExecutionSpaceingauxc/enum.hppGauXC::LoadBalancerFactoryget_shared_instancemethodGauXC::MolecularWeightsFactoryget_instancemethodGauXC::MolecularWeightsSettingsGauXC::functional_type(from ExchCXX)GauXC::XCIntegratorFactoryget_instancemethodGauXC::XCIntegratoreval_exc,eval_exc_vxc, etc. methodsCloses #171