[GRSPH] Conservative to primitive variable swap module#1657
[GRSPH] Conservative to primitive variable swap module#1657y-lapeyre wants to merge 12 commits intoShamrock-code:mainfrom
Conversation
|
Thanks @y-lapeyre 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 @y-lapeyre, 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 a fundamental module for general relativistic hydrodynamics, enabling the conversion of conservative variables to primitive variables. This conversion is essential for advancing simulations, particularly at the end of each time step, by providing the necessary primitive state for subsequent calculations. The implementation leverages an iterative approach based on enthalpy, enhancing the accuracy and robustness of the simulation framework. 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 introduces a new module for converting conservative to primitive variables for GRSPH simulations. The implementation contains several critical issues that will prevent it from compiling or working correctly. These include the use of undefined variables within a kernel, a faulty iterative solver with incorrect logic and missing physics, and out-of-bounds memory access. Additionally, the new module appears to be added to the wrong build target in CMake. I've provided detailed comments and suggestions to address these problems.
| Tscal sqrt_g = get_sqrtg(gcov); | ||
| Tscal inv_sqrt_g = 1. / sqrt_g; | ||
| Tscal sqrt_gamma = get_sqrt_gamma(gcov); | ||
| Tscal sqrt_gamma_inv = alpha * inv_sqrt_g; |
There was a problem hiding this comment.
The variables gcov and alpha are used here but are not defined or captured in the kernel lambda. This will cause a compilation error. The same issue exists for betadown and gammaijUP later in the kernel (lines 106 and 109). These metric-related variables need to be available within the kernel's scope, for example by capturing them in the lambda.
src/shammodels/sph/include/shammodels/sph/modules/ConsToPrim.hpp
Outdated
Show resolved
Hide resolved
|
I can't put the metric as an edge, the comma in the <> is perceived, and the preprocessor freaks out claiming too many arguments are declared. |
You can be declaring an alias, i've done recently https://github.com/tdavidcl/Shamrock/blob/16ed3de43aa5cafea01af0b3306599df71a7b5bb/src/shammodels/ramses/include/shammodels/ramses/modules/SumFluxHydro.hpp. Just declare something like template<class Tvec, class TgridVec>
class NodeSumFluxHydro : public shamrock::solvergraph::INode {
using Tscal = shambase::VecComponent<Tvec>;
using AMRBlock = shammodels::amr::AMRBlock<Tvec, TgridVec, 1>;
u32 block_size;
public:
NodeSumFluxHydro(u32 block_size) : block_size(block_size) {}
+ using CellGraphEdge = solvergraph::OrientedAMRGraphEdge<Tvec, TgridVec>;
EXPAND_NODE_EDGES(NODE_SUM_FLUX_HYDRO)
void _impl_evaluate_internal();
inline virtual std::string _impl_get_label() const { return "SumFluxHydro"; }
inline virtual std::string _impl_get_tex() const { return "TODO"; };
};and use it for the class name |
Workflow reportworkflow report corresponding to commit d38d045 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
|
2 participants
The evaluation of primitive variables is necessary at the end of each time step. Following Tejeda 2012, we express these in terms of the enthalpy w and use the equation of state to find the appropriate value of w through an iterative process.
This PR depends on #1552.