[Ramses] allow setting field values in bulk (with numba)#1676
[Ramses] allow setting field values in bulk (with numba)#1676tdavidcl wants to merge 1 commit 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 focuses on optimizing the process of setting field values within the Ramses model by introducing a bulk assignment mechanism. This change allows Python functions, particularly those optimized with Numba, to compute and apply field values for entire blocks of cells simultaneously, rather than individually. The result is a significant reduction in computation time, enhancing the overall efficiency of the simulation setup. 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 includes changes to the run_kh.py example, such as importing numba, increasing the grid resolution, and modifying physical parameters. It also introduces the use of @njit decorator to accelerate the computation of rho_map, rhovel_map, and rhoetot_map functions. Additionally, the code defines new functions rho_map_all, rhovel_map_all, and rhoetot_map_all to compute the density, velocity, and total energy density, respectively, using all cells. The C++ code in Model.hpp was modified to include a new function set_field_value_lambda_all that takes a lambda function to set field values for all cells. The pyRamsesModel.cpp file was updated to include a utility class VecToNumpy for converting vectors to NumPy arrays, and to expose the set_field_value_lambda_all function to Python. The reviewer noted a critical bug in rhoetot_map_all where the return statement is inside the for loop, causing premature exit and incorrect results, and suggested using NumPy vectorization for performance and bug resolution. The reviewer also suggested replacing the for loop in rho_map_all with np.where for efficiency and readability, and to avoid redundant calculations of rho by calculating it once and passing it as an argument to rhovel_map_all and rhoetot_map_all.
| ncells = in_min_cell.shape[0] | ||
| rhoekin = np.zeros(ncells) | ||
|
|
||
| for i in range(ncells): | ||
| x, y, z = in_min_cell[i] | ||
|
|
||
| rhovel2 = ( | ||
| rhovel[i][0] * rhovel[i][0] | ||
| + rhovel[i][1] * rhovel[i][1] | ||
| + rhovel[i][2] * rhovel[i][2] | ||
| ) | ||
| rhoekin[i] = 0.5 * rhovel2 / rho[i] | ||
|
|
||
| if y > y_interface: | ||
| P = P_2 | ||
| else: | ||
| P = P_1 | ||
|
|
||
| return rhoekin + P / (gamma - 1) |
There was a problem hiding this comment.
This function contains a critical bug: the return statement is inside the for loop, which will cause it to exit after the first iteration with partially computed data. This leads to incorrect results.
Additionally, the function can be fully vectorized using NumPy to improve performance and readability, which also resolves the bug. The current implementation also performs redundant calculations by calling rho_map_all and rhovel_map_all.
y = in_min_cell[:, 1]
rhovel2 = np.sum(rhovel**2, axis=1)
rhoekin = 0.5 * rhovel2 / rho
P = np.where(y > y_interface, P_2, P_1)
return rhoekin + P / (gamma - 1)| rho = np.zeros(ncells) | ||
| for i in range(ncells): | ||
| x, y, z = in_min_cell[i] | ||
| if y > y_interface: | ||
| rho[i] = rho_2 | ||
| else: | ||
| rho[i] = rho_1 |
There was a problem hiding this comment.
The for loop used to populate the rho array can be replaced by a more idiomatic and efficient vectorized NumPy operation. Using np.where will improve both performance and code readability.
y = in_min_cell[:, 1]
rho = np.where(y > y_interface, rho_2, rho_1)| return rho | ||
|
|
||
| def rhovel_map_all(in_min_cell: np.ndarray, in_max_cell: np.ndarray) -> np.array: | ||
| rho = rho_map_all(in_min_cell, in_max_cell) |
There was a problem hiding this comment.
This function recalculates rho by calling rho_map_all, which is inefficient, especially since rhoetot_map_all will also call this function, leading to redundant computations. Consider calculating rho once and passing it as an argument if these functions are to be used together.
Additionally, the loop over ncells can be vectorized using NumPy for significant performance improvements, which aligns with the goal of using numba.
Workflow reportworkflow report corresponding to commit 230b202 Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Doxygen diff with
|
1 participant
To be cleaned but that's the idea