[Stagging PR] Phantom emulation mode #1578
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 significantly expands Shamrock's capabilities for integrating with and emulating Phantom SPH simulations, providing a more streamlined and robust workflow for users working with Phantom data. A core focus was placed on optimizing distributed data communication through a new caching and multi-buffer approach, which is seamlessly integrated across various simulation components. These changes collectively aim to improve performance, flexibility, and user experience when running complex astrophysical simulations. 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 a significant new feature: a Phantom emulation mode. This includes Python utilities for loading and running Phantom simulations, along with an example script. On the C++ side, there's a major refactoring of the sparse communication layer to support multiple buffers and caching, which should improve performance for large messages. The changes are extensive and well-structured. My review focuses on improving the robustness and clarity of the new Python utilities.
|
|
||
| plt.title("t = {:0.3f} [code unit]".format(model.get_time())) | ||
| plt.xlabel("x") | ||
| plt.ylabel("z") |
There was a problem hiding this comment.
The y-axis of the plot is labeled "z", but the view seems to be in the x-y plane, as indicated by delta_y = (0.0, radius * 2, 0.0). For a face-on view of a disc integrated along the z-axis, the y-axis label should be "y".
| plt.ylabel("z") | |
| plt.ylabel("y") |
|
|
||
| # phantom dumps are 00000.tmp if before start and then sim_name_{:05d} | ||
| # parse the dump number | ||
| dump_number = int(dump_file_name.split("_")[1].split(".")[0]) |
There was a problem hiding this comment.
The current parsing of dump_number using split('_') is fragile and will fail if the simulation name (sim_name) contains underscores. Using rsplit('_', 1) would be more robust as it splits from the right, separating the dump number from the simulation name reliably.
| dump_number = int(dump_file_name.split("_")[1].split(".")[0]) | |
| dump_number = int(dump_file_name.rsplit("_", 1)[1].split(".")[0]) |
| with open(os.path.join(simulation_folder, f"{sim_name}.in"), "w") as f: | ||
| for line in lines: | ||
| if "dumpfile" in line: | ||
| line = f" dumpfile = {get_ph_dump_file_name(dump_number)} ! dump file to start from\n" |
There was a problem hiding this comment.
The line to update the dumpfile in the input file has hardcoded indentation. This can alter the file's formatting if the original indentation is different. It's better to preserve the original indentation of the line and only replace the value.
| line = f" dumpfile = {get_ph_dump_file_name(dump_number)} ! dump file to start from\n" | |
| line = f"{line[:len(line) - len(line.lstrip())]}dumpfile = {get_ph_dump_file_name(dump_number)} ! dump file to start from\n" |
Workflow reportworkflow report corresponding to commit 3a81a4d 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
No description provided.