[SPH] add cubic lattice & some exemple about lattice stability #1506
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 enhances the SPH simulation framework by adding the capability to generate cubic crystal lattices for particle initialization. This new feature is supported by core C++ implementations for lattice generation and is seamlessly integrated into the SPH setup module, with Python bindings provided for user convenience. The changes also include illustrative example scripts that showcase the application of these lattice structures in analyzing SPH pairing instability and overall lattice stability. 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. 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 introduces support for cubic lattices within the SPH simulation framework, complete with a new lattice generator and the necessary Python bindings. Additionally, it adds two example scripts that demonstrate the use of both cubic and HCP lattices for investigating lattice stability and pairing instability. The C++ implementation is well-integrated and logically sound. The Python examples are thorough, but they could be enhanced by adhering more strictly to Python's style conventions, particularly concerning the placement of imports and the elimination of redundant code. I have provided specific suggestions to address these points.
|
|
||
| xm, ym, zm = bmin | ||
| xM, yM, zM = bmax | ||
| vol_b = (xM - xm) * (yM - ym) * (zM - zm) |
There was a problem hiding this comment.
The variable vol_b is calculated here but is not used before being recalculated on line 90 with the same values. This line is redundant and can be removed to improve code clarity.
| print(f"hpart min={min_hpart} max={max_hpart} delta={max_hpart-min_hpart}") | ||
|
|
||
| # Compute all pairwise distances | ||
| from scipy.spatial.distance import pdist |
There was a problem hiding this comment.
Imports should be at the top of the file, as per the PEP 8 style guide. Placing imports inside functions (make_plot, show_image_sequence) is inefficient as the module might be re-imported multiple times, and it makes it harder to see the script's dependencies at a glance. Please move from scipy.spatial.distance import pdist (line 161), import matplotlib.animation as animation (line 233), import glob (line 242), and from PIL import Image (line 246) to the top of the script.
References
- Imports should be at the top of the file, as per the PEP 8 style guide. (link)
|
|
||
| xm, ym, zm = bmin | ||
| xM, yM, zM = bmax | ||
| vol_b = (xM - xm) * (yM - ym) * (zM - zm) |
There was a problem hiding this comment.
The variable vol_b is calculated here but is not used before being recalculated on line 100 with the same values. This line is redundant and can be removed to improve code clarity.
| print(f"hpart min={min_hpart} max={max_hpart} delta={max_hpart-min_hpart}") | ||
|
|
||
| # Compute all pairwise distances | ||
| from scipy.spatial.distance import pdist |
There was a problem hiding this comment.
Imports should be at the top of the file, as per the PEP 8 style guide. Placing imports inside functions (make_plot, show_image_sequence) is inefficient as the module might be re-imported multiple times, and it makes it harder to see the script's dependencies at a glance. Please move from scipy.spatial.distance import pdist (line 171), import matplotlib.animation as animation (line 243), import glob (line 252), and from PIL import Image (line 256) to the top of the script.
References
- Imports should be at the top of the file, as per the PEP 8 style guide. (link)
| coord_min[0] = box_min[0]; | ||
| coord_max[0] = box_max[0]; | ||
|
|
||
| coord_min[1] = box_min[1]; | ||
| coord_max[1] = box_max[1]; | ||
|
|
||
| coord_min[2] = box_min[2]; | ||
| coord_max[2] = box_max[2]; |
There was a problem hiding this comment.
This block of code can be simplified. If Tvec supports assignment, you can replace these lines with coord_min = box_min; and coord_max = box_max; for better readability.
| coord_min[0] = box_min[0]; | |
| coord_max[0] = box_max[0]; | |
| coord_min[1] = box_min[1]; | |
| coord_max[1] = box_max[1]; | |
| coord_min[2] = box_min[2]; | |
| coord_max[2] = box_max[2]; | |
| coord_min = box_min; | |
| coord_max = box_max; |
| // std::array<i32, 3> current = { | ||
| // coord_min[0] + i32(current_idx / (coord_delta[1] * coord_delta[2])), | ||
| // coord_min[1] + i32((current_idx / coord_delta[2]) % coord_delta[1]), | ||
| // coord_min[2] + i32(current_idx % coord_delta[2]), | ||
| // }; |
There was a problem hiding this comment.
This commented-out code block appears to be dead code and should be removed to improve readability.
| current[1] = remapped_indices[1][current[1]]; | ||
| current[2] = remapped_indices[2][current[2]]; | ||
|
|
||
| // logger::raw_ln(current, current_idx, max_coord); |
There was a problem hiding this comment.
This commented-out logging statement should be removed from production code to keep it clean.
| static auto init_gen(Tscal dr, std::pair<Tvec, Tvec> box) { | ||
|
|
||
| auto [idxs_min, idxs_max] = Lattice::get_box_index_bounds(dr, box.first, box.second); | ||
| u32 idx_gen = 0; |
There was a problem hiding this comment.
The variable idx_gen is declared and initialized but never used. It should be removed.
| u32 len = pos_data.size(); | ||
| PatchDataField<Tvec> &f | ||
| = tmp.get_field<Tvec>(sched.pdl().get_field_idx<Tvec>("xyz")); | ||
| // sycl::buffer<Tvec> buf(pos_data.data(), len); |
There was a problem hiding this comment.
This commented-out line of code should be removed to improve code clarity.
Workflow reportworkflow report corresponding to commit ac03681 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportNo relevant changes found. You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review. Doxygen diff with
|
1 participant
No description provided.