[AMR][RefinementHandler] Main PR with sod and amr tests#1719
[AMR][RefinementHandler] Main PR with sod and amr tests#1719Akos299 wants to merge 22 commits intoShamrock-code:mainfrom
Conversation
|
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, 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}
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)); |
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 { |
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.
| 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) |
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]) |
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.
| 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}; | ||
| } |
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++) { |
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, |
There was a problem hiding this comment.
The p parameter in the RefineCritPseudoGradientAccessor constructor is unused. Please remove it.
| Tscal err_min, | |
| u64 id_patch, |
| u64 id_patch, | ||
| shamrock::patch::Patch p, | ||
| shamrock::patch::PatchDataLayer &pdat, | ||
| Tscal err_min, |
There was a problem hiding this comment.
The p parameter in the RefineCritPseudoGradientAccessor::finalize method is unused. Please remove it.
| Tscal err_min, | |
| u64 id_patch, |
| err_min = 0.25 | ||
| err_max = 0.15 |
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.
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.
Akos299
force-pushed
the
features/all-amr-dev
branch
from
0f59064 to
cc83d20
Compare
Akos299
force-pushed
the
features/all-amr-dev
branch
from
cc83d20 to
9f73d96
Compare
Save the beautifull AMR
Workflow reportworkflow report corresponding to commit 8cb10b7 Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportSome failures were detected in base source checks checks. ❌ trailing-whitespace❌ ruff-formatSuggested changesDetailed changes :diff --git a/examples/TO_MIGRATE/ramses/sod_amr_thesis.py b/examples/TO_MIGRATE/ramses/sod_amr_thesis.py
index 42152416..2b3dacb7 100644
--- a/examples/TO_MIGRATE/ramses/sod_amr_thesis.py
+++ b/examples/TO_MIGRATE/ramses/sod_amr_thesis.py
@@ -11,6 +11,7 @@ ctx.pdata_layout_new()
model = shamrock.get_Model_Ramses(context=ctx, vector_type="f64_3", grid_repr="i64_3")
+
def run_case(nb_blocks, amr_lev, case="amr"):
multx = 1
multy = 1
@@ -31,11 +32,10 @@ def run_case(nb_blocks, amr_lev, case="amr"):
cfg.set_boundary_condition("z", "reflective")
cfg.set_riemann_solver_hllc()
-
cfg.set_slope_lim_minmod()
cfg.set_face_time_interpolation(True)
- if(case == "amr"):
+ if case == "amr":
err_min = 0.25
err_max = 0.15
cfg.set_amr_mode_pseudo_gradient_based(error_min=err_min, error_max=err_max)
@@ -47,7 +47,6 @@ def run_case(nb_blocks, amr_lev, case="amr"):
(0, 0, 0), (cell_size, cell_size, cell_size), (base * multx, base * multy, base * multz)
)
-
def rho_map(rmin, rmax):
x, y, z = rmin
if x < 0.5:
@@ -55,11 +54,9 @@ def run_case(nb_blocks, amr_lev, case="amr"):
else:
return 0.125
-
etot_L = 1.0 / (gamma - 1)
etot_R = 0.1 / (gamma - 1)
-
def rhoetot_map(rmin, rmax):
x, y, z = rmin
if x < 0.5:
@@ -67,16 +64,13 @@ def run_case(nb_blocks, amr_lev, case="amr"):
else:
return etot_R
-
def rhovel_map(rmin, rmax):
return (0, 0, 0)
-
model.set_field_value_lambda_f64("rho", rho_map)
model.set_field_value_lambda_f64("rhoetot", rhoetot_map)
model.set_field_value_lambda_f64_3("rhovel", rhovel_map)
-
def convert_to_cell_coords(dic):
cmin = dic["cell_min"]
@@ -117,7 +111,6 @@ def run_case(nb_blocks, amr_lev, case="amr"):
return dic
-
t_target = 0.245
dt = 0
@@ -198,20 +191,22 @@ def run_case(nb_blocks, amr_lev, case="amr"):
## density
ax00_1 = axs[0, 0]
- if(case == "amr"):
+ if case == "amr":
ax00_2 = ax00_1.twinx()
ax00_2.set_ylim(np.floor(l.min()), np.ceil(l.max())) # optional but important
ax00_2.yaxis.set_major_locator(ticker.MultipleLocator(1))
ax00_2.yaxis.set_major_formatter(ticker.FormatStrFormatter("%d"))
- ax00_1.scatter(X, rho, rasterized=True, s=12 * np.ones(X.shape), color="red", label="numeric")
+ ax00_1.scatter(
+ X, rho, rasterized=True, s=12 * np.ones(X.shape), color="red", label="numeric"
+ )
ax00_1.plot(arr_x, arr_rho, ls="--", lw=2.0, color="black", label="exact")
ax00_1.set_xlabel("$X$")
ax00_1.set_ylabel("density")
ax00_1.legend(loc=0)
ax00_1.grid()
- if(case == "amr"):
+ if case == "amr":
idx = np.argsort(X)
ax00_2.plot(X[idx], l[idx], color="purple", marker="*", linewidth=1.0, ls="-.")
ax00_2.set_ylabel("AMR level")
@@ -219,7 +214,7 @@ def run_case(nb_blocks, amr_lev, case="amr"):
## velocity
ax01_1 = axs[0, 1]
- if(case=="amr"):
+ if case == "amr":
ax01_2 = ax01_1.twinx()
ax01_2.set_ylim(np.floor(l.min()), np.ceil(l.max())) # optional but important
ax01_2.yaxis.set_major_locator(ticker.MultipleLocator(1))
@@ -232,7 +227,7 @@ def run_case(nb_blocks, amr_lev, case="amr"):
ax01_1.legend(loc=0)
ax01_1.grid()
- if(case=="amr"):
+ if case == "amr":
idx = np.argsort(X)
ax01_2.plot(X[idx], l[idx], color="purple", marker="*", linewidth=1.0, ls="-.")
ax01_2.set_ylabel("AMR level")
@@ -240,7 +235,7 @@ def run_case(nb_blocks, amr_lev, case="amr"):
## pressure
ax11_1 = axs[1, 1]
- if(case =="amr"):
+ if case == "amr":
ax11_2 = ax11_1.twinx()
ax11_2.set_ylim(np.floor(l.min()), np.ceil(l.max())) # optional but important
ax11_2.yaxis.set_major_locator(ticker.MultipleLocator(1))
@@ -254,8 +249,8 @@ def run_case(nb_blocks, amr_lev, case="amr"):
ax11_1.set_ylabel("pressure")
ax11_1.legend(loc=0)
ax11_1.grid()
-
- if(case == "amr"):
+
+ if case == "amr":
idx = np.argsort(X)
ax11_2.plot(X[idx], l[idx], color="purple", marker="*", linewidth=1.0, ls="-.")
ax11_2.set_ylabel("AMR level")
@@ -263,14 +258,19 @@ def run_case(nb_blocks, amr_lev, case="amr"):
## internal energy
ax10_1 = axs[1, 0]
- if(case=="amr"):
+ if case == "amr":
ax10_2 = ax10_1.twinx()
ax10_2.set_ylim(np.floor(l.min()), np.ceil(l.max())) # optional but important
ax10_2.yaxis.set_major_locator(ticker.MultipleLocator(1))
ax10_2.yaxis.set_major_formatter(ticker.FormatStrFormatter("%d"))
ax10_1.scatter(
- X, internal_energy, rasterized=True, s=12 * np.ones(X.shape), color="red", label="numeric"
+ X,
+ internal_energy,
+ rasterized=True,
+ s=12 * np.ones(X.shape),
+ color="red",
+ label="numeric",
)
ax10_1.plot(
arr_x, arr_P / (arr_rho * (gamma - 1.0)), ls="--", lw=2.0, color="black", label="exact"
@@ -280,7 +280,7 @@ def run_case(nb_blocks, amr_lev, case="amr"):
ax10_1.legend(loc=0)
ax10_1.grid()
- if(case=="amr"):
+ if case == "amr":
idx = np.argsort(X)
ax10_2.plot(X[idx], l[idx], color="purple", marker="*", linewidth=1.0, ls="-.")
ax10_2.set_ylabel("AMR level")
@@ -289,4 +289,4 @@ def run_case(nb_blocks, amr_lev, case="amr"):
plt.savefig(f"sod-{case}.pdf")
-run_case(16, 3,"uniform")
+run_case(16, 3, "uniform")
|
2 participants
This PR is the main from which modular ones will be extract from.
sedov_amr_clean_v1.mp4