[RAMSES][AMR] Histogram of AMR Levels and New Block Index Map#1632
[RAMSES][AMR] Histogram of AMR Levels and New Block Index Map#1632Akos299 wants to merge 34 commits intoShamrock-code:mainfrom
Conversation
[Ramses][Solvergraph] AMR level zero sizes Added a solvergraph node to compute level zero sizes for each patch. This will be used for instance to compute the amr level for blocks in each patch. updates
New PR to compute amr levels.
|
Thanks @Akos299 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: |
Summary of ChangesHello @Akos299, 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 significantly enhances the Adaptive Mesh Refinement (AMR) system by implementing the capability to compute a histogram of AMR levels and generate a consistent block index map for reordering data. It introduces a new computational step to accurately determine the base cell size for level 0, which is critical for the AMR level calculations. These new functionalities are seamlessly integrated into the solver's graph, with their execution dynamically controlled by the AMR mode configuration, ensuring computations are only performed when necessary. Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to compute a histogram of AMR levels and a corresponding block index map. The changes include adding new compute nodes to the solver graph for AMR level computation and level 0 cell size, and a new histogramming function.
My review focuses on improving code clarity, performance, and removing unused code:
- In
SolverConfig.hpp, I've suggested renaming a boolean flag for better readability. - In
ComputeLevel0CellSize.hpp, I found an unused variable and an unnecessary computation that can be removed. - In
AMRGridRefinementHandler.cpp, I've identified a performance bottleneck in the new histogramming function and suggested a more efficient parallel approach using a scan operation, aligning with best practices for scan accumulator types.
Overall, the changes are a good addition, and with these improvements, the code will be more robust and performant.
src/shammodels/ramses/include/shammodels/ramses/modules/ComputeLevel0CellSize.hpp
Outdated
Show resolved
Hide resolved
1. Replaced sycl::memory_scope::system by sycl::memory_scope::device 2. Compute offset using exclusive scan
src/shammodels/ramses/include/shammodels/ramses/modules/ComputeLevel0CellSize.hpp
Show resolved
Hide resolved
|
I think This one is ready to be merged. |
for more information, see https://pre-commit.ci
src/shammodels/ramses/include/shammodels/ramses/SolverConfig.hpp
Outdated
Show resolved
Hide resolved
src/shammodels/ramses/include/shammodels/ramses/SolverConfig.hpp
Outdated
Show resolved
Hide resolved
src/shammodels/ramses/include/shammodels/ramses/SolverConfig.hpp
Outdated
Show resolved
Hide resolved
src/shammodels/ramses/include/shammodels/ramses/modules/ComputeLevel0CellSize.hpp
Outdated
Show resolved
Hide resolved
src/shammodels/ramses/include/shammodels/ramses/modules/AMRGridRefinementHandler.hpp
Outdated
Show resolved
Hide resolved
…eLevel0CellSize.hpp
…dRefinementHandler.hpp
for more information, see https://pre-commit.ci
Workflow reportworkflow report corresponding to commit 2e96796 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
This function computes a histogram of AMR (Adaptive Mesh Refinement) levels.
It returns :
The index map can be used to remap existing block-based data to match the new AMR-level ordering.