-
Notifications
You must be signed in to change notification settings - Fork 31
Basic C API for GauXC #172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
dd396c2 to
5881a76
Compare
1d61b38 to
83841ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| * @brief Create a new XCIntegratorFactory instance. | ||
| * @param status Status object to capture any errors. | ||
| * @param execution_space Execution space to use. | ||
| * @param integrator_input_type Type of integrator input. |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter name integrator_input_type is inconsistent with the actual usage. The parameter is documented as "Type of integrator input" but the actual C++ constructor template parameter is for the matrix type (CMatrix). Consider renaming to matrix_type or updating the documentation to reflect that this is the matrix type specification.
| try { | ||
| mol.ptr = new Molecule(); | ||
| status->code = 0; |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The status->message field is not initialized to nullptr in successful code paths. When status->code is set to 0, status->message should also be explicitly set to nullptr (or NULL in C) to ensure consistent state. This affects all functions that use the status pattern throughout the C API.
| #ifdef __cplusplus | ||
| } // extern "C" | ||
| } // namespace GauXC::C | ||
| #endif |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HDF5 include guard checks for GAUXC_HAS_HDF5 but this file is only included when HDF5 is enabled (based on the CMakeLists.txt). The guard on line 13 provides defense in depth, which is good practice, but consider whether the entire file should error out or provide stub implementations when HDF5 is not available, rather than silently compiling to nothing.
| #endif | |
| #endif | |
| #else | |
| #error "GauXC was built without HDF5 support, but <gauxc/external/hdf5.h> was included. Enable GAUXC_HAS_HDF5 to use this header." |
| GauXCMolGrid gauxc_molgrid_new_default( | ||
| GauXCStatus* status, | ||
| const GauXCMolecule mol, | ||
| const enum GauXC_PruningScheme pruning_scheme, | ||
| const int64_t batchsize, | ||
| const enum GauXC_RadialQuad radial_quad, | ||
| const enum GauXC_AtomicGridSizeDefault grid_size | ||
| ) { | ||
| GauXCMolGrid mg{}; | ||
| mg.ptr = nullptr; | ||
|
|
||
| try { | ||
| auto grid_map = MolGridFactory::create_default_gridmap( | ||
| *detail::get_molecule_ptr(mol), | ||
| static_cast<PruningScheme>(pruning_scheme), | ||
| static_cast<BatchSize>(batchsize), | ||
| static_cast<RadialQuad>(radial_quad), | ||
| static_cast<AtomicGridSizeDefault>(grid_size) | ||
| ); | ||
| mg.ptr = new MolGrid(grid_map); | ||
| status->code = 0; | ||
| } catch (std::exception& e) { | ||
| status->code = 1; | ||
| status->message = detail::strdup(e.what()); | ||
| } | ||
| return mg; |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GauXCMolGrid struct is missing the hdr field initialization before returning it. In lines 32-33, mg.ptr is set to nullptr but mg.hdr is never initialized. This should be initialized similar to other factory functions, e.g., mg.hdr = GauXCHeader{GauXC_Type_MolGrid}; before line 36 (in the try block after successful creation).
| void resize(size_t rows, size_t cols) { | ||
| if (rows == rows_ && cols == cols_) return; | ||
| delete[] data_; | ||
| rows_ = rows; | ||
| cols_ = cols; | ||
| data_ = new value_type[rows * cols](); | ||
| } |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the resize method, if rows == 0 or cols == 0, the allocation new value_type[rows * cols]() will allocate a zero-sized array (or array of size 0), which could lead to implementation-defined behavior. Consider explicitly handling the zero-size case by setting data_ to nullptr and not allocating.
| SphericalType{shell.pure}, | ||
| alpha, coeff, O, normalize }; | ||
|
|
||
| if (shell.shell_tolerance >= 0.0) { |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing boundary check for shell tolerance. The condition if (shell.shell_tolerance >= 0.0) should use > instead of >= since a tolerance of exactly 0.0 might not be a valid/meaningful threshold. Alternatively, if 0.0 is a valid value, this should be documented.
| if (shell.shell_tolerance >= 0.0) { | |
| if (shell.shell_tolerance > 0.0) { |
| void gauxc_matrix_resize( | ||
| GauXCStatus* status, | ||
| const GauXCMatrix matrix, | ||
| const size_t rows, | ||
| const size_t cols | ||
| ) { | ||
| status->code = 0; | ||
| try { // can throw std::bad_alloc | ||
| detail::get_matrix_ptr(matrix)->resize( rows, cols ); | ||
| } catch (std::exception& e) { | ||
| status->code = 1; | ||
| status->message = detail::strdup(e.what()); | ||
| } | ||
| } |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter matrix should be marked as const since the function only reads from the matrix and doesn't modify it. This applies to the function declarations in the header as well (lines 66-70 in matrix.h).
| try { | ||
| 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 | ||
|
|
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for the case where the functional name is not found in ExchCXX::functional_map and EXCHCXX_ENABLE_LIBXC is not defined. In this case, the func variable would be uninitialized, leading to undefined behavior when passed to new functional_type. Add an else clause to throw an exception with a clear error message.
| inline static void gauxc_objects_delete( | ||
| GauXCStatus* status, | ||
| void** ptrs, | ||
| size_t nptrs | ||
| ) { | ||
| status->code = 0; | ||
| for(void** ptr = ptrs; ptr < ptrs + nptrs; ++ptr) { | ||
| #ifdef __cplusplus | ||
| if(*ptr != nullptr) { | ||
| #else | ||
| if(*ptr != NULL) { | ||
| #endif | ||
| gauxc_object_delete( status, ptr ); | ||
| if(status->code != 0) return; | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function gauxc_objects_delete is defined as inline static in a header file. This can lead to linker errors when compiling in C mode if multiple translation units include this header, as each will have its own definition of the function. For C compatibility, consider either: (1) removing the inline keyword and defining the function in a source file, or (2) keeping it inline but ensuring it's only called from one translation unit, or (3) using appropriate guards to handle C vs C++ compilation differences.
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