Enh/gamg coarsening #166
Enh/gamg coarsening #166
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the GAMG (Geometric Algebraic Multigrid) coarsening functionality with significant refactoring and restructuring. The changes focus on improving the distributed matrix update system and preconditioner architecture.
- Refactored the distributed matrix update system to handle advanced update scenarios with separate diagonal and face weights
- Restructured preconditioner code by extracting specialized classes for Jacobi and Multigrid preconditioners
- Enhanced RepartDistMatrix with cloning capabilities and improved data structure management
Reviewed Changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/MatrixWrapper/Distributed.cpp | Major refactoring of update system, added advanced_update_impl function, and modified data structures to include offset information |
| include/OGL/lduLduBase.hpp | Updated preconditioner initialization to pass ExecutorHandler instead of device executor |
| include/OGL/Solver/Multigrid.hpp | Removed extra blank line formatting cleanup |
| include/OGL/Preconditioner/Schwarz.hpp | New file defining Schwarz preconditioner wrapper functions |
| include/OGL/Preconditioner/PreconditionerWrapper.hpp | New base class for preconditioner wrappers |
| include/OGL/Preconditioner/Multigrid.hpp | New dedicated Multigrid class extracted from main Preconditioner class |
| include/OGL/Preconditioner/Jacobi.hpp | New dedicated BlockJacobi class extracted from main Preconditioner class |
| include/OGL/Preconditioner.hpp | Significantly simplified by extracting functionality to specialized classes and commenting out unused code |
| include/OGL/MatrixWrapper/Distributed.hpp | Enhanced with clone functionality and helper methods for updating data pointers |
| CMakeLists.txt | Updated Ginkgo version from ogl_0600_gko190 to ogl_0600_gko110 |
Comments suppressed due to low confidence (1)
include/OGL/Preconditioner/Multigrid.hpp:1
- Spelling error: 'coarseWeigth' should be 'coarseWeight'.
// SPDX-FileCopyrightText: 2025 OGL authors
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| get_id = (start_offset == 0) ? -1 : 0; | ||
| } | ||
| recv_ptr = linops[get_id] + offset; | ||
| recv_ptr = linops[get_id]; // + offset; |
There was a problem hiding this comment.
[nitpick] This commented-out addition of offset suggests incomplete refactoring. Either remove the comment if the offset is now handled elsewhere, or implement the offset addition if it's still needed.
| recv_ptr = linops[get_id]; // + offset; | |
| recv_ptr = linops[get_id] + offset; |
Copilot uses AI. Check for mistakes.
| // << "\n"; | ||
| update_data.push_back(RepartDistMatrix::pairwise_data{ | ||
| linop_id, mode, comm_rank, length, send_id, recv_ptr}); | ||
| linop_id, mode, comm_rank, length, send_id, recv_ptr, offset}); |
There was a problem hiding this comment.
The addition of offset parameter changes the structure signature. Ensure all code using this structure is updated to handle the new parameter correctly.
Copilot uses AI. Check for mistakes.
| auto rank = exec_handler.get_host_rank(); | ||
| auto device_exec = exec_handler.get_device_exec(); | ||
| bool force_host_buffer = exec_handler.get_gko_force_host_buffer(); | ||
| word fieldname = "foo"; |
There was a problem hiding this comment.
[nitpick] Using a hardcoded string 'foo' as fieldname is not descriptive. Consider using a more meaningful name or making it configurable.
| word fieldname = "foo"; | |
| word fieldname = "all_to_all_update"; |
Copilot uses AI. Check for mistakes.
| std::vector<RepartDistMatrix::pairwise_data> &pairwise_update_data, | ||
| std::vector<RepartDistMatrix::reorder_map_type> &reorder_maps, bool fuse, | ||
| std::map<label, scalar *> linops, label verbose) | ||
| label verbose) |
There was a problem hiding this comment.
The removal of the linops parameter from update_impl function signature indicates a significant API change. Verify that all callers have been updated accordingly.
Copilot uses AI. Check for mistakes.
| std::vector<reorder_map_type> in, scalar *new_ptr) const | ||
| { | ||
| std::vector<reorder_map_type> out; | ||
| auto [map, _, pad] = in[0]; //) { |
There was a problem hiding this comment.
The commented code fragment '//) {' appears to be leftover from refactoring and should be removed.
| auto [map, _, pad] = in[0]; //) { | |
| auto [map, _, pad] = in[0]; |
Copilot uses AI. Check for mistakes.
No description provided.