[SPH][Phys] Add Tillotson EoS #1445
Conversation
Summary of ChangesHello @DavidFang03, 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 the Tillotson Equation of State (EoS) into the 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 the Tillotson Equation of State (EoS), a feature useful for high-velocity impact simulations. The changes span across the configuration, serialization, core physics implementation, and Python bindings. The implementation is comprehensive, adding the necessary logic for the new EoS in various parts of the system.
My review focuses on improving code quality, correctness, and maintainability. I've identified a critical bug in an equality operator that misses some fields, which could lead to incorrect behavior. I've also pointed out several areas for code cleanup and improvement, such as removing redundant code, fixing typos in documentation, completing a TODO, and improving code style for better readability. These suggestions will help ensure the new code is robust and easy to maintain.
| auto [p, dpdrho, dpdu] = compute_cold(); | ||
| P = p; | ||
| dP_drho = dpdrho; | ||
| dP_du = dpdu; | ||
| } else if (u > u_cv) { | ||
| auto [p, dpdrho, dpdu] = compute_hot(); | ||
| P = p; | ||
| dP_drho = dpdrho; | ||
| dP_du = dpdu; |
There was a problem hiding this comment.
The blocks for the if and else if conditions contain identical logic for unpacking the tuple and assigning values to P, dP_drho, and dP_du. This can be simplified by using std::tie to assign values directly to these variables, making the code more concise and less repetitive.
std::tie(P, dP_drho, dP_du) = compute_cold();
} else if (u > u_cv) {
std::tie(P, dP_drho, dP_du) = compute_hot();
} else {| * @tparam Tscal Scalar type | ||
| * | ||
| * The equation of state is given by: | ||
| * TODO |
There was a problem hiding this comment.
The documentation for EOS_Config_Tillotson contains a TODO marker. This should be filled in with the equation of state to provide complete documentation for users of this struct.
Workflow reportworkflow report corresponding to commit 62818e2 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. ❌ Authorship update requiredThe following files had their author headers updated by the author update script. Please run the script again ( Note: The list below is only partial. Only the first 10 files are shown.
Suggested changesDetailed changes :diff --git a/src/shampylib/src/pyShamphys.cpp b/src/shampylib/src/pyShamphys.cpp
index dedbe873..59132730 100644
--- a/src/shampylib/src/pyShamphys.cpp
+++ b/src/shampylib/src/pyShamphys.cpp
@@ -9,6 +9,7 @@
/**
* @file pyShamphys.cpp
+ * @author David Fang (david.fang@ikmail.com)
* @author Timothée David--Cléris (tim.shamrock@proton.me)
* @brief
*/
|
2 participants
Tillotson EoS was originally designed for high velocity impacts [Tillotson 1962] and has been extended to geologic materials, for example [Brundage 2013]
Usage: