[Examples] fix dustysod IC & improve dustywave plots - #1949
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: |
📝 WalkthroughWalkthroughThe PR updates two SPH dusty TVA examples: configurable dusty-wave sizing and plotting, plus revised Sod-tube dust initialization, timescale, labels, and figure outputs. ChangesDusty-wave example
Dusty TVA Sod-tube example
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/sph/run_dustywave_tva.py`:
- Around line 50-51: Update the lmin and lmax bounds calculation in the box
setup to preserve odd configured dimensions such as LZ=19; use floating-point
half-length bounds so the domain spans the requested size, or explicitly
validate and reject odd values instead of silently rounding them to the
even-sized domain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c3ac773d-ee40-4f1d-8a58-fc6699ceb64c
📒 Files selected for processing (2)
examples/sph/run_dustywave_tva.pyexamples/sph/run_sod_dust_tva.py
| lmin = (-(lx // 2), -(ly // 2), -(lz // 2)) | ||
| lmax = (lx // 2, ly // 2, lz // 2) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve odd configured box lengths.
With LZ=19, these bounds are identical to LZ=18 (-9 to 9), so the requested domain size is silently ignored. Use half-length floating bounds, or explicitly reject odd values.
Proposed fix
-lmin = (-(lx // 2), -(ly // 2), -(lz // 2))
-lmax = (lx // 2, ly // 2, lz // 2)
+lmin = (-lx / 2, -ly / 2, -lz / 2)
+lmax = (lx / 2, ly / 2, lz / 2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lmin = (-(lx // 2), -(ly // 2), -(lz // 2)) | |
| lmax = (lx // 2, ly // 2, lz // 2) | |
| lmin = (-lx / 2, -ly / 2, -lz / 2) | |
| lmax = (lx / 2, ly / 2, lz / 2) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/sph/run_dustywave_tva.py` around lines 50 - 51, Update the lmin and
lmax bounds calculation in the box setup to preserve odd configured dimensions
such as LZ=19; use floating-point half-length bounds so the domain spans the
requested size, or explicitly validate and reject odd values instead of silently
rounding them to the even-sized domain.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/sph/run_dustywave_tva.py (1)
480-507: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard GIF encoding to rank 0.
show_image_sequencereturnsNoneon nonzero MPI ranks, but the addedani1.save(...)andani2.save(...)calls are unconditional. Multi-rank runs will therefore fail with an attribute error; guard all animation creation/saving with a rank-0 check. (github.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/sph/run_dustywave_tva.py` around lines 480 - 507, Guard the ani1 and ani2 animation creation and save blocks, including their associated show_image_sequence calls, behind the existing rank-0 condition so nonzero MPI ranks do not call save on None. Keep the plotting flow unchanged for rank 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@examples/sph/run_dustywave_tva.py`:
- Around line 480-507: Guard the ani1 and ani2 animation creation and save
blocks, including their associated show_image_sequence calls, behind the
existing rank-0 condition so nonzero MPI ranks do not call save on None. Keep
the plotting flow unchanged for rank 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6bd5d693-981a-496d-a971-f8b038ad4cfa
📒 Files selected for processing (1)
examples/sph/run_dustywave_tva.py
Workflow reportworkflow report corresponding to commit 16874be 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
No description provided.