Open
Conversation
This reorganization will help decouple components. For example, the vmec field needs to use interpolation. With the utility libraries moved to a lower level, we can avoid a circular dependency between pcms::core pcms::interpolator. That is, pcms::core can utilize pcms::interpolator. This is really the first step in a broader re-organization of the pcms::sources to be more modular.
We create a library that performs localization tasks. At the moment, we only have one strategy, i.e., use a unform grid for localization. However, we anticipate adding additional backends for AroborX (SCOREC#245), adjacency based, KD-Tree, etc. This library split helps to organize the code, move things out of pcms::core which initally had everything, and clarify dependencies.
There was a problem hiding this comment.
Pull request overview
This PR continues the reorganization of the PCMS codebase by extracting utility and localization components into separate libraries (pcms_utility and pcms_localization). This modularization improves code organization and enables better dependency management.
- Moves utility headers (arrays, types, print, assert, etc.) from
pcms/topcms/utility/subdirectory with a new CMake library target - Extracts point search functionality into a new
pcms_localizationlibrary - Updates all include paths throughout the codebase to reflect the new structure
- Adds missing includes (e.g.,
<cmath>in coordinate_transform.h) that were previously satisfied transitively - Prefixes internal helper macros with
PCMS_to avoid potential naming conflicts
Reviewed changes
Copilot reviewed 59 out of 63 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pcms/utility/CMakeLists.txt | Defines new pcms_utility library with proper exports and include paths |
| src/pcms/localization/CMakeLists.txt | Defines new pcms_localization library linking to pcms_utility and Omega_h |
| src/pcms/localization/point_search.cpp | Implements point search algorithms (newly added implementation file) |
| src/pcms/utility/*.h, *.cpp | Utility headers/sources moved with updated internal includes |
| src/pcms/utility/types.h | Removes redev dependency, adds standard library includes |
| src/pcms/utility/assert.h | Prefixes helper macros (CAT, SELECT, etc.) with PCMS_ namespace |
| src/pcms/coupler.h | Adds ProcessType alias to shield users from redev types |
| src/pcms/coordinate_transform.h | Adds missing <cmath> include for trigonometric functions |
| src/CMakeLists.txt | Removes utility source files, adds subdirectories and library dependencies |
| config.cmake.in | Includes new library target exports for utility and localization |
| test/.cpp, tools/.cpp, examples/*.cpp | Updates includes to use new pcms/utility/ and pcms/localization/ paths |
Comments suppressed due to low confidence (1)
src/pcms/utility/assert.cpp:1
- Missing include for
abort()function. The functionabort()is called on line 8 but the required header<cstdlib>is not included. While the code may compile in some environments due to transitive includes, this is fragile and may break in other environments or with different compilers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5
to
+7
| #include "pcms/utility/arrays.h" | ||
| #include "field.h" | ||
| #include "pcms/arrays.h" | ||
| #include "pcms/utility/arrays.h" |
There was a problem hiding this comment.
Duplicate include directive. The header "pcms/utility/arrays.h" is included twice (lines 5 and 7). Remove one of the duplicate includes to keep the code clean.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This PR is the second step of the reorganization started in #252. It should be reviewed after #252.