Skip to content

Deactivate out of bounds particle#1000

Open
LasNikas wants to merge 23 commits intotrixi-framework:mainfrom
LasNikas:deactivate-particles-outside-domain
Open

Deactivate out of bounds particle#1000
LasNikas wants to merge 23 commits intotrixi-framework:mainfrom
LasNikas:deactivate-particles-outside-domain

Conversation

@LasNikas
Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread src/general/neighborhood_search.jl Outdated
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 84.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.16%. Comparing base (a3f1139) to head (92bbc66).

Files with missing lines Patch % Lines
src/general/neighborhood_search.jl 81.25% 3 Missing ⚠️
test/general/neighborhood_search.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1000      +/-   ##
==========================================
- Coverage   89.17%   89.16%   -0.02%     
==========================================
  Files         128      129       +1     
  Lines        9925     9950      +25     
==========================================
+ Hits         8851     8872      +21     
- Misses       1074     1078       +4     
Flag Coverage Δ
total 89.16% <84.00%> (-0.02%) ⬇️
unit 67.60% <64.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@svchb
Copy link
Copy Markdown
Collaborator

svchb commented Nov 28, 2025

PR 1000 🎉🔥

Comment thread src/general/neighborhood_search.jl Outdated
@LasNikas LasNikas marked this pull request as ready for review January 7, 2026 15:41
@LasNikas LasNikas requested review from efaulhaber and svchb January 7, 2026 15:41
@LasNikas LasNikas self-assigned this Jan 7, 2026
@LasNikas
Copy link
Copy Markdown
Collaborator Author

LasNikas commented Jan 9, 2026

Any thoughts on this, @efaulhaber and @svchb ? I can't think of anything better at the moment. However, I can't see any reason why we shouldn't have this (except for performance, but I think that's negligible?)

@LasNikas LasNikas force-pushed the deactivate-particles-outside-domain branch from 9fb29f5 to e3eac7b Compare January 10, 2026 11:29
@LasNikas
Copy link
Copy Markdown
Collaborator Author

/run-gpu-tests

@svchb
Copy link
Copy Markdown
Collaborator

svchb commented Jan 19, 2026

Any thoughts on this, @efaulhaber and @svchb ? I can't think of anything better at the moment. However, I can't see any reason why we shouldn't have this (except for performance, but I think that's negligible?)

I think it should be optional. Otherwise I am fine with this.

@LasNikas
Copy link
Copy Markdown
Collaborator Author

I think it should be optional. Otherwise I am fine with this.

I also considered making this optional. However, it would then be necessary to explain that this can only be set if a full grid cell list and a system buffer are used. Users should not expect this to apply to other constellations. This would need to be explained in more detail. Should I do that? @svchb @efaulhaber

@svchb
Copy link
Copy Markdown
Collaborator

svchb commented Jan 20, 2026

I think it should be optional. Otherwise I am fine with this.

I also considered making this optional. However, it would then be necessary to explain that this can only be set if a full grid cell list and a system buffer are used. Users should not expect this to apply to other constellations. This would need to be explained in more detail. Should I do that? @svchb @efaulhaber

You could of course implement this for the other nhs types as well.

@LasNikas
Copy link
Copy Markdown
Collaborator Author

You could of course implement this for the other nhs types as well.

But the domain is potentially infinite for the other NHS. Also, it only works with system buffer, as the particles are not deleted but only deactivated.

@svchb
Copy link
Copy Markdown
Collaborator

svchb commented Jan 20, 2026

You could of course implement this for the other nhs types as well.

But the domain is potentially infinite for the other NHS. Also, it only works with system buffer, as the particles are not deleted but only deactivated.

Hmm... You could throw an error message if its activated for other nhs types.

Comment thread src/schemes/boundary/open_boundary/system.jl Outdated
Comment thread src/general/neighborhood_search.jl Outdated
Comment thread test/count_allocations.jl Outdated
@LasNikas LasNikas force-pushed the deactivate-particles-outside-domain branch from 8ac6a42 to e1bceff Compare March 19, 2026 10:15
@LasNikas LasNikas requested a review from efaulhaber March 19, 2026 10:17
@LasNikas LasNikas added enhancement New feature or request and removed discussion labels Mar 19, 2026
@efaulhaber
Copy link
Copy Markdown
Member

Tests are failing.

Copy link
Copy Markdown
Member

@efaulhaber efaulhaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add more comments explaining why this is done and when. Something like "the full grid cell list requires a bounding box. For this cell list, we deactivate particles that are outside the bounding box. These particles are glitched out of the open boundary and don't contribute anyway, and would crash the simulation if they move outside the bounding box and are not removed.

Comment thread src/general/neighborhood_search.jl Outdated
@LasNikas LasNikas requested a review from efaulhaber April 17, 2026 12:33
Comment thread src/general/neighborhood_search.jl
@LasNikas LasNikas requested a review from efaulhaber April 20, 2026 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants