[AMR][RefinementHandler] Main PR with sod and amr tests#1719
Open
Akos299 wants to merge 6 commits intoShamrock-code:mainfrom
Open
[AMR][RefinementHandler] Main PR with sod and amr tests#1719Akos299 wants to merge 6 commits intoShamrock-code:mainfrom
Akos299 wants to merge 6 commits intoShamrock-code:mainfrom
Conversation
This commit is the first in a series leading to a PR that updates the AMR algorithms. Specifically, it implements the 2:1 refinement consistency check.
Contributor
|
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: |
Contributor
Summary of ChangesHello, 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) capabilities of the RAMSES solver by introducing a new pseudo-gradient based refinement criterion. It includes comprehensive C++ backend modifications to support this new mode, alongside new Python examples that validate its functionality for common hydrodynamic test cases. The changes also refine the grid management process by enforcing the 2:1 refinement rule and updating internal data structures to better track and utilize AMR levels. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new pseudo-gradient-based AMR refinement mode, including new PseudoGradientBased structs and related methods in SolverConfig.hpp and AMRGridRefinementHandler.hpp, along with corresponding Python bindings. It also adds new utility functions for pseudo-gradient calculation in SlopeLimitedGradientUtilities.hpp and a rho_primitive field to SolverStorage.hpp and Solver.cpp for use in the new AMR logic. The changes also include updates to ConsToPrimGas to handle this new field. Review comments highlight a critical bug where refine_list is moved multiple times, leading to undefined behavior, and several improvement opportunities such as removing unused parameters and redundant computations, clarifying confusing comments, addressing potential logical inconsistencies in err_min/err_max values, and suggesting more robust convergence checks for iteration loops in the AMR refinement process.
| enforce_two_to_one_refinement(std::move(refine_list)); | ||
|
|
||
| /////// enforce 2:1 for derefinement ////// | ||
| enforce_two_to_one_derefinement(std::move(derefine_list), std::move(refine_list)); |
Contributor
There was a problem hiding this comment.
The refine_list is moved to enforce_two_to_one_refinement on line 1246 and then moved again to enforce_two_to_one_derefinement here. Moving an already moved object results in undefined behavior. If the content of refine_list is still needed after the first move, it should be passed by reference or copied.
src/shammodels/ramses/include/shammodels/ramses/modules/ComputeLevel0CellSize.hpp
Show resolved
Hide resolved
| using namespace sham; | ||
| using namespace sham::details; | ||
|
|
||
| auto get_avg_neigh = [&](auto &graph_links, u32 dir) -> T { |
Contributor
There was a problem hiding this comment.
The u32 dir parameter in the get_avg_neigh lambda is unused. Please remove it to improve code clarity and avoid potential confusion.
Suggested change
| auto get_avg_neigh = [&](auto &graph_links, u32 dir) -> T { | |
| auto get_avg_neigh = [&](auto &graph_links) -> T { |
|
|
||
|
|
||
| def rhovel_map(rmin, rmax): | ||
| rho = rho_map(rmin, rmax) |
Contributor
There was a problem hiding this comment.
The rho variable is calculated here but is not used within the rhovel_map function. This is a redundant computation and can be removed.
| next_dt = model.evolve_once_override_time(t, dt) | ||
| if i == 0: | ||
| dic0 = convert_to_cell_coords(ctx.collect_data()) | ||
| dX0.append(dic0["ymax"][i] - dic0["ymin"][i]) |
Contributor
There was a problem hiding this comment.
This line appends to dX0 inside a loop, but the condition if i == 0 means it will only execute once, causing dX0 to contain a single element repeated many times. If dX0 is meant to store initial cell sizes, it should be populated only once outside the loop, or its purpose needs clarification.
Comment on lines
+282
to
+293
| auto get_coord = [](u32 i) -> std::array<u32, dim> { | ||
| constexpr u32 NsideBlockPow = 1; | ||
| constexpr u32 Nside = 1U << NsideBlockPow; | ||
| constexpr u32 side_size = Nside; | ||
| constexpr u32 block_size = shambase::pow_constexpr<dim>(Nside); | ||
|
|
||
| if constexpr (dim == 3) { | ||
| const u32 tmp = i >> NsideBlockPow; | ||
| // This line is why derefinement never happens | ||
| // return {i % Nside, (tmp) % Nside, (tmp ) >> NsideBlockPow}; | ||
| return {(tmp) >> NsideBlockPow, (tmp) % Nside, i % Nside}; | ||
| } |
Contributor
There was a problem hiding this comment.
The commented-out line // This line is why derefinement never happens is confusing. If this was a bug that has been fixed, the comment should be removed. If it's still a potential issue or a historical note, please clarify its meaning and current relevance.
| // // // enforce 2:1 at parent level | ||
| // /////////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| for (int it = 0; it < 3; it++) { |
Contributor
There was a problem hiding this comment.
The for (int it = 0; it < 3; it++) loop uses a fixed number of iterations for convergence. Similar to the refinement loop, consider adding a convergence check or a more dynamic stopping condition for robustness.
| u64 id_patch, | ||
| shamrock::patch::Patch p, | ||
| shamrock::patch::PatchDataLayer &pdat, | ||
| Tscal err_min, |
Contributor
There was a problem hiding this comment.
The p parameter in the RefineCritPseudoGradientAccessor constructor is unused. Please remove it.
Suggested change
| Tscal err_min, | |
| u64 id_patch, |
| u64 id_patch, | ||
| shamrock::patch::Patch p, | ||
| shamrock::patch::PatchDataLayer &pdat, | ||
| Tscal err_min, |
Contributor
There was a problem hiding this comment.
The p parameter in the RefineCritPseudoGradientAccessor::finalize method is unused. Please remove it.
Suggested change
| Tscal err_min, | |
| u64 id_patch, |
Comment on lines
+37
to
+38
| err_min = 0.25 | ||
| err_max = 0.15 |
Contributor
There was a problem hiding this comment.
Similar to sedo_amr.py, err_min (0.25) is greater than err_max (0.15). This configuration is unusual for refinement/derefinement thresholds and could lead to unexpected behavior. Please confirm if this is the intended logic.
Contributor
Workflow reportworkflow report corresponding to commit 0f59064 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
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 main from which modular ones will be extract from.
sedov_amr_clean_v1.mp4