[GSPH] migrate ghost_cache to solvergraph#1582
Open
Guo-astro wants to merge 2 commits intoShamrock-code:mainfrom
Open
[GSPH] migrate ghost_cache to solvergraph#1582Guo-astro wants to merge 2 commits intoShamrock-code:mainfrom
Guo-astro wants to merge 2 commits intoShamrock-code:mainfrom
Conversation
Add GhostCacheEdge for ghost interface cache management via solvergraph. Changes: - Add GhostCacheEdge.hpp for GSPH ghost cache edge - Add solver_graph and ghost_cache edge to SolverStorage - Register edge in init_solver_graph() - Update build_ghost_cache() to use solvergraph edge - Update clear_ghost_cache() to use free_alloc() - Update all ghost_cache usages to solvergraph pattern Part of GSPH solvergraph migration (1 object per PR).
Summary of ChangesHello @Guo-astro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request is a step in the ongoing migration of the GSPH solver to use the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
|
Thanks @Guo-astro for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a GhostCacheEdge for managing ghost interface caches using the solvergraph. The changes include adding the new edge type, integrating it into SolverStorage, updating the ghost cache building and clearing processes, and modifying existing usages to adopt the solvergraph pattern. The changes look good overall, and the suggestions provided aim to improve code clarity and maintainability.
Comment on lines
+90
to
+93
| // Register ghost cache edge for dependency tracking | ||
| storage.ghost_cache = storage.solver_graph.register_edge( | ||
| "ghost_cache", | ||
| solvergraph::GhostCacheEdge<Tvec>("ghost_cache", "\\mathcal{C}_{\\rm ghost}")); |
There was a problem hiding this comment.
This is good to register the ghost cache edge for dependency tracking. However, consider adding a comment explaining why dependency tracking is important in this context. This will help future developers understand the purpose of this registration.
Also, the name ghost_cache is repeated in the register_edge call. It would be beneficial to define this name as a constant to avoid potential typos and improve maintainability.
Suggested change
| // Register ghost cache edge for dependency tracking | |
| storage.ghost_cache = storage.solver_graph.register_edge( | |
| "ghost_cache", | |
| solvergraph::GhostCacheEdge<Tvec>("ghost_cache", "\\mathcal{C}_{\\rm ghost}")); | |
| // Define a constant for the ghost cache edge name to avoid typos and improve maintainability | |
| constexpr char const* ghost_cache_edge_name = "ghost_cache"; | |
| // Register ghost cache edge for dependency tracking | |
| // Dependency tracking ensures that the ghost cache is valid before being used in computations | |
| storage.ghost_cache = storage.solver_graph.register_edge( | |
| ghost_cache_edge_name, | |
| solvergraph::GhostCacheEdge<Tvec>(ghost_cache_edge_name, "\\mathcal{C}_{\\rm ghost}")); |
Comment on lines
+182
to
+186
| shambase::get_check_ref(storage.ghost_cache) | ||
| .set(gsph_utils.build_interf_cache( | ||
| storage.ghost_handler.get(), | ||
| storage.serial_patch_tree.get(), | ||
| solver_config.htol_up_coarse_cycle)); |
There was a problem hiding this comment.
Using shambase::get_check_ref here is good for ensuring that the ghost_cache is properly initialized before use. However, consider adding a comment explaining why this check is necessary and what the consequences are if the cache is not properly set. This will help future developers understand the importance of this check.
shambase::get_check_ref(storage.ghost_cache)
// get_check_ref ensures that the ghost_cache is properly initialized before use.
// If the ghost_cache is not properly set, it throws an exception to prevent further computation with invalid cache.
.set(gsph_utils.build_interf_cache(
storage.ghost_handler.get(),
storage.serial_patch_tree.get(),
solver_config.htol_up_coarse_cycle));| void shammodels::gsph::Solver<Tvec, Kern>::clear_ghost_cache() { | ||
| StackEntry stack_loc{}; | ||
| storage.ghost_patch_cache.reset(); | ||
| shambase::get_check_ref(storage.ghost_cache).free_alloc(); |
There was a problem hiding this comment.
Similar to the previous comment, it would be helpful to add a comment explaining why shambase::get_check_ref is used here and what the consequences are if the cache is not properly set before freeing the allocated cache.
shambase::get_check_ref(storage.ghost_cache)
// get_check_ref ensures that the ghost_cache is properly initialized before use.
// If the ghost_cache is not properly set, it throws an exception to prevent attempting to free an invalid cache.
.free_alloc();
Contributor
Workflow reportworkflow report corresponding to commit 07e8d67 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
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
2 participants
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.
Summary
Add
GhostCacheEdgefor ghost interface cache management via solvergraph.Part of GSPH solvergraph migration (1 object per PR per maintainer request).
Changes:
GhostCacheEdge.hppfor GSPH ghost cache edgesolver_graphandghost_cacheedge toSolverStorageinit_solver_graph()build_ghost_cache()to use solvergraph edgeclear_ghost_cache()to usefree_alloc()ghost_cacheusages to solvergraph patternTest plan