bug: m_bubbles_EL: delete non-finite bubbles in the boundary enforcement - #1772
bug: m_bubbles_EL: delete non-finite bubbles in the boundary enforcement#1772aliencaocao wants to merge 4 commits into
Conversation
s_enforce_EL_bubbles_boundary_conditions deletes out-of-range bubbles with pos >= x_cb(m) / pos < x_cb(-1) which are both FALSE for a NaN position, so a bubble whose state went non-finite survives, and its NaN mtn_s reaches s_get_cell/s_compute_stddsv, where int(NaN)=INT_MIN indexes dx out of bounds (CUDA_ERROR_ILLEGAL_ADDRESS on GPU at ~step 96). Fold each pair into a single .not.(in-range) guard, bit-identical for finite positions (and +/-Inf, already caught) but also removing NaN. Reproduced and fixed on 80CC6F73 at 200 steps.
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a GPU crash in bubbles_lagrange by ensuring bubbles with non-finite positions (e.g., NaN under strong forcing) are correctly deleted during boundary enforcement, preventing downstream invalid indexing/OOB reads.
Changes:
- Reworks boundary checks to delete bubbles unless their position is inside the valid domain interval.
- Applies the same “inside-range negation” logic consistently for x/y/z boundary enforcement.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Lines of Code
|
|
Hi @sbryngelson wonder why was it marked as draft? this PR is ready as it is from my side |
|
@aliencaocao it's to manage the CI queue for self hosted runners. There have been some instabilities that are being fixed. Drafts keep the runners available for PRs pushing fixes. |
|
Oh ok, thanks for the explanation. I will keep it as draft and leave it to you guys to manage. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1772 +/- ##
=======================================
Coverage 61.67% 61.68%
=======================================
Files 84 84
Lines 21619 21613 -6
Branches 3196 3196
=======================================
- Hits 13334 13331 -3
+ Misses 6093 6090 -3
Partials 2192 2192 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The single bubble's ODE state can go non-finite under the strong acoustic forcing (mag=20000), so mtn_pos becomes NaN. s_enforce_EL_bubbles_boundary_conditions removes out-of-range bubbles with
and both comparisons are false for NaN, so the non-finite bubble is never deleted. Its NaN
mtn_sreachess_get_cell(int(NaN) = INT_MIN), ands_compute_stddsvreadsdx(cell(1))~17 GB out of boundsThis causes all bubbles_lagrange to crash on GPUs when the bubble goes NaN, at different steps. For example, case 80CC6F73 crashes at step 96.
The CI currently does not catch this bug as it is only run for 50 steps. It also will not crash in CPU release mode because OOB read is silent there, and the simulation just proceeds with a corrupted NaN state. To verify on CPU, one need to run with debug mode -Mbound, and it will correctly show the OOB at step 96.
The fix is to flip the check from checking if it is outside to negate(if it is inside), so NaN correctly become false and is deleted
cc @wilfonba as discussed in SC26 SCC google group
Contribution Policy
We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.
All contributions are expected to demonstrate:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
PR template credit: junegunn