-
Notifications
You must be signed in to change notification settings - Fork 10
Add rpath #176
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
Conversation
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 encompasses changes from an old branch with two primary themes: (1) adding rpath configuration to CMake for improved library path handling, particularly for Python bindings, and (2) extensive additions and modifications including new kernel density estimation (KDE) functionality, CUDA implementations, API refactoring, and various bug fixes.
Key changes:
- CMake rpath configuration for library installation paths
- New 2D kernel density estimation implementation (KDE2d) with FFT-based smoothing
- CUDA implementation for UserBar force computation
- API modernization: refactoring gen_point methods from error-code pattern to std::tuple returns
- New utility
addringfor adding particle rings to realizations - Multiple bug fixes, version updates, and code quality improvements
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Critical bug: Missing space in CMAKE_INSTALL_RPATH; version bump to 7.9.0 |
| utils/PhaseSpace/KDE2d.{H,cc} | New 2D kernel density estimation class with FFT convolution; contains buffer overflow bug |
| utils/PhaseSpace/diffpsp.cc | Integration of KDE smoothing with new parameters; inverted conditional logic |
| utils/ICs/gensph.cc | Fibonacci lattice replication algorithm with Euler angles; spelling errors |
| utils/ICs/addring.cc | New ring particle generator; misleading help text, duplicate include, spelling errors |
| utils/ICs/ZangICs.cc | Added particle replication feature per orbit |
| src/user/cudaUserBar.{cu,cc,H} | New CUDA acceleration implementation for bar potential |
| src/user/CMakeLists.txt | Updated build configuration for CUDA sources |
| src/SphericalBasis.{cc,H} | New FIX_L0 feature to freeze monopole coefficients |
| include/massmodel.H | API modernization with PSret tuple type |
| exputil/realize_model.cc | Comprehensive refactoring to use tuple returns instead of error parameters |
| exputil/massmodel_dist.cc | Replaced #define macros with const variables; initialized variables |
| exputil/EmpCyl{SL,2d}.cc | Improved cache validation with diagnostic messages |
| exputil/EXPmath.cc | Converted preprocessor macros to const variables |
| expui/expMSSA.{cc,H} | Fixed type bug: bool attributes instead of double |
| expui/BiorthBasis.cc | Multiple improvements: coefficient caching, non-inertial frame handling, time interpolation fixes |
| expui/BasisFactory.{cc,H} | Enhanced non-inertial coordinate system support with better error handling |
| pyEXP/{UtilWrappers,BasisWrappers}.cc | Python API additions for version info and non-inertial frames; spelling errors |
| include/cudaParticle.cuH | CUDA 12.4+ compatibility for thrust functors |
| doc/exp.cfg | Version update to 7.8.1 (inconsistent with CMakeLists.txt) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
CMakeLists.txt
Outdated
|
|
||
| # Add an explicit path to the install library directory (esp. useful | ||
| # for Python bindings) | ||
| set(CMAKE_INSTALL_RPATH"${CMAKE_INSTALL_PREFIX}/lib") |
Copilot
AI
Nov 25, 2025
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 space between CMAKE_INSTALL_RPATH and the string value. This should be:
set(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib")Without the space, this will create a concatenated variable name rather than setting the intended variable.
| set(CMAKE_INSTALL_RPATH"${CMAKE_INSTALL_PREFIX}/lib") | |
| set(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib") |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Looks like some merge confusion (on my part) in the original branch: the changes to So now it's back to the RPATH change only. I am unsure if we still want/need this but I don't see any harm in it. It sets the runtime search path libraries (and executables) after they have been installed. I usually do this at the environment level might be might help make the path search more automatic. So let's do it. |
These two commits were sitting alone in an old branch when I was doing some maintenance this morning -- not sure if we want or need them.