Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for two new scientific computing libraries: GSL (GNU Scientific Library) and NLopt (nonlinear optimization library). The changes integrate these libraries into the existing C++ Python extension module and provide Python bindings for basic functionality from each library.
- Adds GSL integration with a Bessel function wrapper
- Adds NLopt integration with a basic optimization function
- Updates vcpkg triplet configurations to use static linking instead of dynamic
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_basic.py | Adds test cases for the new GSL Bessel function and NLopt optimization functionality |
| src/cpp/main.cpp | Implements C++ wrapper functions for GSL and NLopt and exposes them via pybind11 |
| CMakeLists.txt | Configures build system to find and link GSL and NLopt libraries from vcpkg |
| .github/workflows/wheels.yml | Updates vcpkg triplet configurations from dynamic to static linking |
| .github/workflows/test.yml | Updates vcpkg triplet configurations from dynamic to static linking |
| project(cppcore) | ||
|
|
||
| set(VCPKG_HOST_TRIPLET $ENV{VCPKG_HOST_TRIPLET}) | ||
| set(VCPKG_HOST_TRIPLET $ENV{VCPKG_HOST_TRIPLET}) |
There was a problem hiding this comment.
This line appears to be a duplicate removal, but the same line is set again on line 5. This creates confusion about whether this is intentional duplication or an incomplete cleanup.
| set(VCPKG_HOST_TRIPLET $ENV{VCPKG_HOST_TRIPLET}) |
| pybind11_add_module(cppcore src/cpp/main.cpp) | ||
|
|
||
| include_directories(${CMAKE_SOURCE_DIR}/src/cpp ${EIGEN_PATH}) | ||
| include_directories(${CMAKE_SOURCE_DIR}/src/cpp ${EIGEN_PATH} ${GSL_INCLUDE_DIR} ${NLOPT_DIR_INCLUDE_DIR}) |
There was a problem hiding this comment.
Using global 'include_directories' is discouraged in modern CMake. Consider using 'target_include_directories' exclusively, which you're already doing on line 31.
| include_directories(${CMAKE_SOURCE_DIR}/src/cpp ${EIGEN_PATH} ${GSL_INCLUDE_DIR} ${NLOPT_DIR_INCLUDE_DIR}) |
No description provided.