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

Particle packing #529

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

Conversation

LasNikas
Copy link
Collaborator

@LasNikas LasNikas commented May 15, 2024

based on #439 and #436

Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 57.81585% with 197 lines in your changes missing coverage. Please review.

Project coverage is 67.84%. Comparing base (e5a8834) to head (384a51f).

Files with missing lines Patch % Lines
src/preprocessing/particle_packing/system.jl 0.00% 117 Missing ⚠️
src/general/initial_condition.jl 7.69% 24 Missing ⚠️
src/preprocessing/particle_packing/rhs.jl 0.00% 18 Missing ⚠️
src/setups/complex_shape.jl 38.88% 11 Missing ⚠️
src/preprocessing/geometries/io.jl 0.00% 9 Missing ⚠️
.../preprocessing/particle_packing/signed_distance.jl 94.65% 7 Missing ⚠️
test/preprocessing/packing/nhs_faces.jl 88.09% 5 Missing ⚠️
src/callbacks/update.jl 0.00% 2 Missing ⚠️
test/preprocessing/packing/signed_distance.jl 33.33% 2 Missing ⚠️
src/general/custom_quantities.jl 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
- Coverage   68.51%   67.84%   -0.68%     
==========================================
  Files          87       93       +6     
  Lines        5406     5852     +446     
==========================================
+ Hits         3704     3970     +266     
- Misses       1702     1882     +180     
Flag Coverage Δ
unit 67.84% <57.81%> (-0.68%) ⬇️

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.

src/preprocessing/geometries/io.jl Outdated Show resolved Hide resolved
src/setups/complex_shape.jl Outdated Show resolved Hide resolved
src/setups/complex_shape.jl Outdated Show resolved Hide resolved
src/setups/complex_shape.jl Outdated Show resolved Hide resolved
Comment on lines +25 to +30
- `is_boundary`: When `is_boundary=true`, boundary particles will be sampled
and packed in an offset surface of the `boundary`.
The thickness of the boundary is specified by passing the
[`SignedDistanceField`](@ref) of `boundary` with:
- `use_for_boundary_packing=true`
- `max_signed_distance=boundary_thickness`
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this description. What is this option doing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If set to true the ParticlePackingSystem packs only boundary particles of the geometry.
If set to false (default) the ParticlePackingSystem packs the particles inside the geometry.

Copy link
Member

Choose a reason for hiding this comment

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

So there is always only one system packed? Either inside or boundary? This is not clear to me from the docs.
And I still don't understand this description.

Suggested change
- `is_boundary`: When `is_boundary=true`, boundary particles will be sampled
and packed in an offset surface of the `boundary`.
The thickness of the boundary is specified by passing the
[`SignedDistanceField`](@ref) of `boundary` with:
- `use_for_boundary_packing=true`
- `max_signed_distance=boundary_thickness`
- `pack_boundary`: When `pack_boundary=false`, the particles in `boundary`
are fixed and the particles in `shape` are packed.
When `pack_boundary=true`, the particles in `shape` are fixed
and the particles in `boundary` are fixed.
It often makes sense to first pack the `shape` against the `boundary`
and then in the next step `boundary` against the packed `shape`.
The thickness of the boundary is specified by passing the
[`SignedDistanceField`](@ref) of `boundary` with:
- `use_for_boundary_packing=true`
- `max_signed_distance=boundary_thickness`

Is this correct? Is it not possible to pack both at the same time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, that's not correct.
I have two different systems. One is filled with the particles inside the geometry (fluid) and one is filled with particles outside the geometry (boundary). These are independent systems that interact with each other.

The only difference between these systems is the shift condition used to constrain particles onto the surface.

For example:

packing_system = ParticlePackingSystem(shape_sampled; boundary=geometry,
                                       signed_distance_field, tlsph=tlsph,
                                       background_pressure)

boundary_system = ParticlePackingSystem(boundary_sampled; boundary=geometry,
                                        is_boundary=true, signed_distance_field,
                                        tlsph=tlsph, background_pressure)

semi = Semidiscretization(packing_system, boundary_system)

src/preprocessing/particle_packing/system.jl Outdated Show resolved Hide resolved
src/preprocessing/particle_packing/system.jl Outdated Show resolved Hide resolved
src/preprocessing/particle_packing/system.jl Outdated Show resolved Hide resolved
src/preprocessing/particle_packing/system.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@svchb svchb left a comment

Choose a reason for hiding this comment

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

This still requires tests.

@LasNikas LasNikas requested a review from efaulhaber December 5, 2024 20:25
Comment on lines +80 to +81
# Initialize neighborhood search with signed distances
PointNeighbors.initialize_grid!(nhs, stack(signed_distance_field.positions))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@efaulhaber should I use a full grid nhs here? The positions of the signed_distance_field are of course static. Or what is the best choice here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, by default the nhs should be using a FullGridCellList. You need to set this in the kwarg.
This cell list is faster and GPU-compatible. Only downsides are 1. one needs to specify a bounding box and 2. the domain is constrained to this bounding box. Here you know the bounding box and particles should never leave it, right?

Copy link
Collaborator Author

@LasNikas LasNikas Dec 11, 2024

Choose a reason for hiding this comment

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

Here you know the bounding box and particles should never leave it, right?

Yes, exactly.

@LasNikas
Copy link
Collaborator Author

LasNikas commented Dec 13, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants