Make LB coupling work with any skin #3025
Conversation
Codecov Report
@@ Coverage Diff @@
## python #3025 +/- ##
=======================================
+ Coverage 84% 84% +<1%
=======================================
Files 526 526
Lines 26089 26091 +2
=======================================
+ Hits 22132 22137 +5
+ Misses 3957 3954 -3
Continue to review full report at Codecov.
|
| * @return True iff the point is inside of the domain. | ||
| */ | ||
| bool in_local_halo(Vector3d const &pos) { | ||
| auto const halo = 0.5 * lb_lbfluid_get_lattice().agrid; |
| void lb_lbcoupling_calc_particle_lattice_ia(bool couple_virtual) { | ||
| void lb_lbcoupling_calc_particle_lattice_ia( | ||
| bool couple_virtual, const ParticleRange &local_particles, | ||
| const ParticleRange &more_particles) { |
There was a problem hiding this comment.
what are more_particles supposed to be? Please add a docstring
There was a problem hiding this comment.
Well propose a better name. I'd prefer just calling the function twice for each range, but that is not possible because of the GPU stuff.
There was a problem hiding this comment.
okay different question: do you think that a dev in 3 years still knows whatmore_particles are supposed to be without looking into the source code?
|
I didn't even notice because on my machine it's skipped because vtk is missing. |
|
I don't get it. I also don't understand why there is a ghost communication for ENGINE right before the coupling. Maybe I don't understand the algorithm. |
|
The halo region might have to include the swimmer->second-force-center distance. |
|
The As as side note, all of this could be avoided if the force centers were just implemented with virtual sites. |
|
As as side note, all of this could be avoided if the force centers were just implemented with virtual sites.
We may have to do this for Walberla in any case. That would probably also make testing this easier.
|
|
Yes, the reason for this PR is that I saw what you were doing for walberla and thought this might help you. |
fweik
changed the title
|
I took another look. I have no idea how to proceed until we have a test for a singe integration step with an engine particle. It is also not clear why the engine gpu and engine cpu tests use different reference data. If I run the engine gpu with the cpu reference data, the error is even bigger than the change cause dby this PR. |
|
@christophlohrmann, based on your observations, should we merge this PR? |
|
I can't speak about other things, but this PR fixes the issue of the missing second force centers when crossing periodic boundaries, so it's definitely better than the current state |
|
The fix even goes so far, that the engine_lb on the cpu can use the same reference data as on the gpu. |
RudolfWeeber
changed the title
|
Originally there was no parallel support. I allowed it at some point because the tests did pass and it should have worked. I did not check the tests though. |
|
@KaiSzuttor, Flo answered the question about more_particles on Aug 10. I suggest to merge as is. |
|
bors r+ |
3025: Make LB coupling work with any skin r=KaiSzuttor a=fweik Description of changes: - Particle coupling forces are now calculated by the node in whose LB domain the particle position is, and not by the node that owns the particle as before. - This now works independent of skin. - Also fixes the wrong sanity check in the LB, before you could use it with an incompatible skin if verlet lists were disabled, which would then crash. Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de> Co-authored-by: Rudolf Weeber <weeber@icp.uni-stuttgart.de> Co-authored-by: RudolfWeeber <weeber@icp.uni-stuttgart.de>
Build succeeded |
Description of changes:
node in whose LB domain the particle position is,
and not by the node that owns the particle as before.
could use it with an incompatible skin if verlet lists were
disabled, which would then crash.