-
-
Notifications
You must be signed in to change notification settings - Fork 8
Adds test suite and benchmarks #69
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
Open
BradyPlanden
wants to merge
18
commits into
main
Choose a base branch
from
testing-suite
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
23741ef
Fix: CasADi linking to work with pip isolated build environments
BradyPlanden 7551a6d
tests: adds initial test suite w/ basic functionality
BradyPlanden 8dd3ea2
infra: adds __version__ to package, adds nox to dev dependencies w/ u…
BradyPlanden 90ca76c
tests: adds benchmark session, uv backend for nox, updates to unit su…
BradyPlanden ebffa02
benchmarks: remove icons
BradyPlanden 9a70d96
Fix: RPath installation bool, updates PYBAMM_ENV in noxfile, adds nox…
BradyPlanden 11cebde
test: try condition RPATH
BradyPlanden 680e502
fix: precommit, integration workflow missing uv
BradyPlanden ccddae1
benchmarks: update regression threshold, increase repeats, try: fix w…
BradyPlanden 046e98e
fix: integration workflow
BradyPlanden 6abe878
CI: unifi package management
BradyPlanden 184383c
benchmarks: timeit implementation
BradyPlanden afd14d3
test: adds casadi-based integration tests, removes simplistic unit te…
BradyPlanden 1e78371
precommit additions
BradyPlanden 728ec40
Suggestions from review
BradyPlanden dc5427f
precommit additions
BradyPlanden a204965
fix: avoid single-element vector operations for MacOS-intel
BradyPlanden accd07f
another test: separate negate from casadi symbol
BradyPlanden File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
name: Benchmarks | ||
|
||
on: | ||
pull_request: | ||
branches: | ||
- "main" | ||
|
||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.ref }} | ||
cancel-in-progress: true | ||
|
||
jobs: | ||
pytest: | ||
runs-on: ${{ matrix.os }} | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [macos-latest] | ||
python-version: ["3.12"] | ||
|
||
steps: | ||
- uses: actions/checkout@v5 | ||
with: | ||
submodules: 'recursive' | ||
|
||
- name: Install dependencies (macOS) | ||
if: matrix.os == 'macos-15-intel' || matrix.os == 'macos-latest' | ||
env: | ||
HOMEBREW_NO_INSTALL_CLEANUP: 1 | ||
HOMEBREW_NO_AUTO_UPDATE: 1 | ||
HOMEBREW_NO_COLOR: 1 | ||
NONINTERACTIVE: 1 | ||
run: | | ||
brew analytics off | ||
brew install libomp | ||
brew reinstall gcc | ||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v6 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
|
||
- name: Install nox | ||
run: | | ||
pip install nox | ||
- name: Run benchmarks | ||
run: | | ||
nox -s benchmarks |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,3 +16,7 @@ build | |
# Extra directories for local work | ||
workspace | ||
deploy | ||
PyBaMM | ||
|
||
# benchmark results | ||
performance_results.json |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,25 +137,51 @@ if (${USE_PYTHON_CASADI}) | |
message("Found Python CasADi library directory: ${CASADI_LIB_DIR}") | ||
target_link_directories(idaklu PRIVATE ${CASADI_LIB_DIR}) | ||
|
||
set_target_properties( | ||
idaklu PROPERTIES | ||
INSTALL_RPATH "${CASADI_LIB_DIR}" | ||
INSTALL_RPATH_USE_LINK_PATH TRUE | ||
) | ||
# Attempt to link the casadi library directly if found | ||
find_library(CASADI_LIBRARY NAMES casadi PATHS ${CASADI_LIB_DIR} NO_DEFAULT_PATH) | ||
if (CASADI_LIBRARY) | ||
message("Found CasADi library: ${CASADI_LIBRARY}") | ||
target_link_libraries(idaklu PRIVATE ${CASADI_LIBRARY}) | ||
else () | ||
message(WARNING "CasADi library not found in ${CASADI_LIB_DIR}. The target will rely on transitive linkage via CMake config if available.") | ||
endif () | ||
# Set RPATH to find libraries relative to the module location | ||
# This allows finding casadi in the same Python environment at runtime | ||
# Module is at: site-packages/pybammsolvers/idaklu.so | ||
# CasADi is at: site-packages/casadi/libcasadi.dylib | ||
# SuiteSparse/SUNDIALS are found via DYLD_LIBRARY_PATH/LD_LIBRARY_PATH | ||
# (set by noxfile or user environment) pointing to .idaklu/lib | ||
# Note: Windows uses vcpkg with static linking, no RPATH needed | ||
|
||
# For CI wheel builds, use BUILD_WITH_INSTALL_RPATH=FALSE so that | ||
# wheel repair tools (delocate/auditwheel) can properly analyze dependencies. | ||
# For local development, use BUILD_WITH_INSTALL_RPATH=TRUE so the module | ||
# works immediately after pip install without wheel repair. | ||
if(DEFINED ENV{CIBUILDWHEEL}) | ||
set(USE_INSTALL_RPATH_AT_BUILD FALSE) | ||
else() | ||
set(USE_INSTALL_RPATH_AT_BUILD TRUE) | ||
endif() | ||
|
||
if (APPLE) | ||
set_target_properties( | ||
idaklu PROPERTIES | ||
BUILD_RPATH "${CASADI_LIB_DIR}" | ||
BUILD_RPATH_USE_LINK_PATH FALSE | ||
INSTALL_RPATH "@loader_path/../casadi" | ||
BUILD_WITH_INSTALL_RPATH ${USE_INSTALL_RPATH_AT_BUILD} | ||
) | ||
else() | ||
set_target_properties( | ||
idaklu PROPERTIES | ||
BUILD_RPATH "${CASADI_LIB_DIR}" | ||
BUILD_RPATH_USE_LINK_PATH FALSE | ||
INSTALL_RPATH "$ORIGIN/../casadi" | ||
BUILD_WITH_INSTALL_RPATH ${USE_INSTALL_RPATH_AT_BUILD} | ||
) | ||
endif() | ||
# Link against casadi by name, not absolute path, to avoid issues with | ||
# pip's isolated build environments changing paths between configure and build | ||
target_link_libraries(idaklu PRIVATE casadi) | ||
Comment on lines
+175
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All good catches here, thanks! and sorry for the trouble here, I realise I was responsible for all this 😄 |
||
else () | ||
message(FATAL_ERROR "Could not find CasADi library directory") | ||
endif () | ||
else () | ||
message("Trying to link against any casadi package apart from the Python one") | ||
find_package(casadi CONFIG REQUIRED) | ||
target_link_libraries(idaklu PRIVATE casadi) | ||
endif () | ||
|
||
# openmp | ||
|
@@ -180,7 +206,7 @@ set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PROJECT_SOURCE_DIR}) | |
find_package(SUNDIALS REQUIRED) | ||
message("SUNDIALS found in ${SUNDIALS_INCLUDE_DIR}: ${SUNDIALS_LIBRARIES}") | ||
target_include_directories(idaklu PRIVATE ${SUNDIALS_INCLUDE_DIR}) | ||
target_link_libraries(idaklu PRIVATE ${SUNDIALS_LIBRARIES} casadi) | ||
target_link_libraries(idaklu PRIVATE ${SUNDIALS_LIBRARIES}) | ||
|
||
# link suitesparse | ||
# if using vcpkg, use config mode to | ||
|
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.