[Doc] add Toro shock tests examples to sphinx#1646
[Doc] add Toro shock tests examples to sphinx#1646tdavidcl wants to merge 4 commits intoShamrock-code:mainfrom
Conversation
|
Thanks @tdavidcl 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 @tdavidcl, 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 introduces new documentation examples for the 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
|
![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 adds a new example script for running Toro shock tests. The script is well-structured, but there are several opportunities for improvement in terms of code clarity, maintainability, and adherence to Python best practices. I've provided suggestions to refactor if/elif chains into dictionaries, remove unused code, improve efficiency in numerical computations, and streamline repetitive code blocks. These changes will make the example easier to read and maintain.
| if slope_limiter == "none": | ||
| cfg.set_slope_lim_none() | ||
| elif slope_limiter == "vanleer": | ||
| cfg.set_slope_lim_vanleer_f() | ||
| elif slope_limiter == "vanleer_std": | ||
| cfg.set_slope_lim_vanleer_std() | ||
| elif slope_limiter == "vanleer_sym": | ||
| cfg.set_slope_lim_vanleer_sym() | ||
| elif slope_limiter == "minmod": | ||
| cfg.set_slope_lim_minmod() | ||
| else: | ||
| raise ValueError(f"Invalid slope limiter: {slope_limiter}") |
There was a problem hiding this comment.
This if/elif chain for selecting the slope limiter can be refactored to use a dictionary that maps names to methods. This improves readability and makes it easier to add new limiters in the future.
slope_limiter_map = {
"none": cfg.set_slope_lim_none,
"vanleer": cfg.set_slope_lim_vanleer_f,
"vanleer_std": cfg.set_slope_lim_vanleer_std,
"vanleer_sym": cfg.set_slope_lim_vanleer_sym,
"minmod": cfg.set_slope_lim_minmod,
}
if slope_limiter in slope_limiter_map:
slope_limiter_map[slope_limiter]()
else:
raise ValueError(f"Invalid slope limiter: {slope_limiter}")| if riemann_solver == "rusanov": | ||
| cfg.set_riemann_solver_rusanov() | ||
| elif riemann_solver == "hll": | ||
| cfg.set_riemann_solver_hll() | ||
| elif riemann_solver == "hllc": | ||
| cfg.set_riemann_solver_hllc() | ||
| else: | ||
| raise ValueError(f"Invalid Riemann solver: {riemann_solver}") |
There was a problem hiding this comment.
Similar to the slope limiter selection, this if/elif chain for the Riemann solver can be replaced with a dictionary for better maintainability and readability.
riemann_solver_map = {
"rusanov": cfg.set_riemann_solver_rusanov,
"hll": cfg.set_riemann_solver_hll,
"hllc": cfg.set_riemann_solver_hllc,
}
if riemann_solver in riemann_solver_map:
riemann_solver_map[riemann_solver]()
else:
raise ValueError(f"Invalid Riemann solver: {riemann_solver}")| etot_R = p_R / (gamma - 1) + 0.5 * rho_R * vx_R**2 | ||
|
|
||
| def rhoetot_map(rmin, rmax): | ||
| rho = rho_map(rmin, rmax) |
There was a problem hiding this comment.
The variable rho is assigned but never used within the rhoetot_map function. This line should be removed to improve code clarity.
| return etot_R | ||
|
|
||
| def rhovel_map(rmin, rmax): | ||
| rho = rho_map(rmin, rmax) |
There was a problem hiding this comment.
The variable rho is assigned but never used within the rhovel_map function. This line should be removed.
| vx = np.array(rhov_vals)[:, 0] / np.array(rho_vals) | ||
| P = (np.array(rhoetot_vals) - 0.5 * np.array(rho_vals) * vx**2) * (gamma - 1) | ||
| results_dic = { | ||
| "rho": np.array(rho_vals), | ||
| "vx": np.array(vx), | ||
| "P": np.array(P), | ||
| } |
There was a problem hiding this comment.
To improve efficiency and readability, you can convert the lists to NumPy arrays once at the beginning of the calculations. Also, np.array() is called on variables that are already NumPy arrays, which is redundant.
rho_vals_np = np.array(rho_vals)
vx = np.array(rhov_vals)[:, 0] / rho_vals_np
P = (np.array(rhoetot_vals) - 0.5 * rho_vals_np * vx**2) * (gamma - 1)
results_dic = {
"rho": rho_vals_np,
"vx": vx,
"P": P,
}| for i in range(3): | ||
| axes[i].set_xlabel("$x$") | ||
| # axes[i].set_yscale("log") | ||
| axes[i].grid(True, alpha=0.3) | ||
|
|
||
| ax1, ax2, ax3 = axes | ||
| ax1.set_xlabel("$x$") | ||
| ax1.set_ylabel("$\\rho$") | ||
|
|
||
| ax2.set_xlabel("$x$") | ||
| ax2.set_ylabel("$v_x$") | ||
|
|
||
| ax3.set_xlabel("$x$") | ||
| ax3.set_ylabel("$P$") |
There was a problem hiding this comment.
The setup for the plot axes can be simplified. Currently, you are setting the x-label multiple times. You can use a single loop to configure all axes, making the code more concise.
ax1, ax2, ax3 = axes
y_labels = ["$\\rho$", "$v_x$", "$P$"]
for ax, y_label in zip(axes, y_labels):
ax.set_xlabel("$x$")
ax.set_ylabel(y_label)
# ax.set_yscale("log")
ax.grid(True, alpha=0.3)|
|
||
| arr_x = [x[0] for x in positions_plot] | ||
|
|
||
| import matplotlib.animation as animation |
There was a problem hiding this comment.
According to PEP 8, imports should usually be at the top of the file. Please move import matplotlib.animation as animation to the top of the file with the other imports. This makes the code's dependencies clearer.
| run_and_plot(cases, 1, "minmod_hll") | ||
| run_and_plot(cases, 2, "minmod_hll") | ||
| run_and_plot(cases, 3, "minmod_hll") | ||
| run_and_plot(cases, 4, "minmod_hll") | ||
| run_and_plot(cases, 5, "minmod_hll") | ||
| run_and_plot(cases, 6, "minmod_hll") | ||
| run_and_plot(cases, 7, "minmod_hll") |
There was a problem hiding this comment.
The repeated calls to run_and_plot for different test numbers can be simplified by using a loop. This makes the code more concise and easier to modify if the number of tests changes.
for test_number in range(1, 8):
run_and_plot(cases, test_number, "minmod_hll")
Workflow reportworkflow report corresponding to commit 34dca00 Pre-commit check reportSome failures were detected in base source checks checks. ❌ ruff-formatSuggested changesDetailed changes :diff --git a/doc/sphinx/examples/ramses/run_toro_shocks.py b/doc/sphinx/examples/ramses/run_toro_shocks.py
index 4695888c..f434d4b0 100644
--- a/doc/sphinx/examples/ramses/run_toro_shocks.py
+++ b/doc/sphinx/examples/ramses/run_toro_shocks.py
@@ -315,18 +315,18 @@ def run_and_plot(cases, test_number, case_anim):
# %%
cases = [
- #("none", "rusanov"),
- #("none", "hll"),
- #("none", "hllc"),
- #("minmod", "rusanov"),
- #("minmod", "hll"),
+ # ("none", "rusanov"),
+ # ("none", "hll"),
+ # ("none", "hllc"),
+ # ("minmod", "rusanov"),
+ # ("minmod", "hll"),
("minmod", "hllc"),
]
cases_no_hllc = [
- #("none", "rusanov"),
- #("none", "hll"),
- #("minmod", "rusanov"),
+ # ("none", "rusanov"),
+ # ("none", "hll"),
+ # ("minmod", "rusanov"),
("minmod", "hll"),
]
|
1 participant
No description provided.