[SPH] Hard wall boundary condition #1558
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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 lays the groundwork for implementing hard wall boundary conditions by introducing a flexible configuration mechanism for defining particle disabling regions. It integrates a particle selection process into the main solver loop, allowing for dynamic identification of particles based on these defined regions. However, there is a notable inconsistency where the module intended to identify particles 'in wall' actually selects particles 'outside' a spherical boundary, and the solver's current implementation explicitly uses an 'outside sphere' module, which might require clarification or adjustment depending on the exact definition of a 'hard wall boundary condition' in this context. 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 mechanism for a 'hard wall boundary condition' by disabling particles. While the goal is clear, the implementation has several significant issues with naming consistency, code duplication, and potentially incorrect logic. A new ParticleDisableConfig is added, but its Wall struct actually defines a sphere. A new module, GetParticlesInWall, is introduced but it's a functional duplicate of GetParticlesOutsideSphere, is misnamed, and contains logic that selects particles outside the region. Furthermore, the solver logic incorrectly uses the old GetParticlesOutsideSphere module instead of the new one. I've left several comments to address these critical design and correctness issues to improve the clarity and maintainability of the code.
src/shammodels/sph/include/shammodels/sph/modules/GetParticlesInWall.hpp
Show resolved
Hide resolved
tdavidcl
reviewed
Jan 19, 2026
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
i kind of like the beginning of it.
I would recommend setting sph force to 0 after update dervis so that you don't need to pass extra things to it.
Also could you rename ParticleDisablingConfig to something like GhostParticleConfig for consistency with Phantom ?
Collaborator
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a hard wall boundary condition by disabling particles within specified wall regions. The implementation has several critical issues that need to be addressed. There's a logic error in the Python bindings for adding a wall, an inverted condition for checking if walls exist, and incomplete JSON serialization for wall configurations. Additionally, there's an out-of-bounds access in get_edges and some leftover debug logs. All comments provided are valid and do not contradict any of the established rules. I've provided detailed comments and suggestions to fix these issues.
|
|
||
| update_derivs(); | ||
|
|
||
| bool has_walls = solver_config.particle_disable.disable_list.empty(); |
There was a problem hiding this comment.
The logic for has_walls is inverted. disable_list.empty() returns true if the list is empty, meaning has_walls will be true when there are no walls. The particle disabling logic inside the if (has_walls) block will only execute when no walls are configured. You should negate the result of empty().
Suggested change
| bool has_walls = solver_config.particle_disable.disable_list.empty(); | |
| bool has_walls = !solver_config.particle_disable.disable_list.empty(); |
src/shammodels/sph/include/shammodels/sph/modules/GetParticlesInWall.hpp
Outdated
Show resolved
Hide resolved
tdavidcl
reviewed
Jan 21, 2026
Contributor
Workflow reportworkflow report corresponding to commit 3388cdb 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
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.