Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch default for foreach_point_neighbor #687

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

svchb
Copy link
Collaborator

@svchb svchb commented Dec 17, 2024

Using each_moving_particle as default leads to hard to debug issues especially in multi system simulations.

@svchb svchb self-assigned this Dec 17, 2024
@svchb svchb requested review from efaulhaber and LasNikas December 17, 2024 15:54
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.71%. Comparing base (b91c603) to head (d4031a2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #687      +/-   ##
==========================================
- Coverage   68.75%   68.71%   -0.04%     
==========================================
  Files          93       93              
  Lines        5840     5840              
==========================================
- Hits         4015     4013       -2     
- Misses       1825     1827       +2     
Flag Coverage Δ
unit 68.71% <100.00%> (-0.04%) ⬇️

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.

Copy link
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.

Can you please also remove points=eachparticle(system) for the corrections and the boundary stuff, since it's the default now?

examples/n_body/n_body_system.jl Outdated Show resolved Hide resolved
@@ -11,7 +11,11 @@ function interact!(dv, v_particle_system, u_particle_system,
# Loop over all pairs of particles and neighbors within the kernel cutoff.
foreach_point_neighbor(particle_system, neighbor_system,
system_coords, neighbor_coords,
neighborhood_search) do particle, neighbor, pos_diff, distance
neighborhood_search;
points=each_moving_particle(particle_system)) do particle,
Copy link
Member

Choose a reason for hiding this comment

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

Redundant. All particles are moving.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could be not the case I think if they are deactivated?

neighborhood_search) do particle, neighbor,
pos_diff, distance
neighborhood_search;
points=each_moving_particle(system)) do particle, neighbor,
Copy link
Member

Choose a reason for hiding this comment

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

See above.

@@ -103,7 +103,9 @@ function calc_normal_akinci!(system, neighbor_system::FluidSystem,

foreach_point_neighbor(system, neighbor_system,
system_coords, neighbor_system_coords,
neighborhood_search) do particle, neighbor, pos_diff, distance
neighborhood_search;
points=each_moving_particle(system)) do particle, neighbor,
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? What about fixed boundary particles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It depends but in this model it is assumed that the boundary particles don't belong to the same species.

@@ -176,7 +176,9 @@ function update!(density_diffusion::DensityDiffusionAntuono, neighborhood_search
set_zero!(normalized_density_gradient)

foreach_point_neighbor(system, system, system_coords, system_coords,
neighborhood_search) do particle, neighbor, pos_diff, distance
neighborhood_search;
points=each_moving_particle(system)) do particle, neighbor,
Copy link
Member

Choose a reason for hiding this comment

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

See above.

@@ -22,7 +22,11 @@ function interact!(dv, v_particle_system, u_particle_system,
# Loop over all pairs of particles and neighbors within the kernel cutoff.
foreach_point_neighbor(particle_system, neighbor_system,
system_coords, neighbor_system_coords,
neighborhood_search) do particle, neighbor, pos_diff, distance
neighborhood_search;
points=each_moving_particle(particle_system)) do particle,
Copy link
Member

Choose a reason for hiding this comment

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

See above.

@svchb svchb requested a review from efaulhaber January 20, 2025 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants