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

LB coupling for RegularDecomposition without ghost shifts. #4470

Merged
merged 10 commits into from
Apr 20, 2022

Conversation

pkreissl
Copy link
Contributor

@pkreissl pkreissl commented Mar 10, 2022

Fixes #4466, fixes #4432, closes #4440

Description of changes:

  • ports n-square lb coupling support from the walberla branch back into the python branch
  • merges PR WIP: Do not shift ghost positions in domain decomposition #4440 and fixes CI issues along the way
  • expose some funtions in lb_particle_coupling.hpp to get rid of code duplication in (the fixed version of) lb_inertialess_tracers.cpp

@pkreissl pkreissl force-pushed the lb_coupling_no_ghost_shifts branch from 3fc9829 to 5a871c7 Compare March 10, 2022 10:25
@pkreissl pkreissl force-pushed the lb_coupling_no_ghost_shifts branch from f5aac6e to 12cdd7c Compare March 10, 2022 13:20
@pkreissl pkreissl force-pushed the lb_coupling_no_ghost_shifts branch from b59bd6e to 64d59a7 Compare March 10, 2022 15:13
@RudolfWeeber
Copy link
Contributor

@pkreissl, what's the status. Given that this Pr is high-risk and the feature freeze is fast aproaching.

@pkreissl pkreissl force-pushed the lb_coupling_no_ghost_shifts branch 3 times, most recently from bf421c7 to 6d65b4e Compare April 9, 2022 13:02
@pkreissl pkreissl force-pushed the lb_coupling_no_ghost_shifts branch from 6d65b4e to 813210a Compare April 9, 2022 13:08
@pkreissl
Copy link
Contributor Author

pkreissl commented Apr 9, 2022

Resolving the code duplication in the LB coupling of both, regular particles and inertialess tracers, proved to be not quite as straight-forward as anticipated. I am open for suggestions for improvement.

  • In lb_inertialess_tracers: former in_local_domain() was basically the same as in_local_halo() of lb_particle_coupling.cpp
  • In the current commit inline in_local_domain() is defined in both .cpp files. Putting it into the lb_particle_coupling.hpp is not directly possible, as the function requires an include "grid.hpp". This would lead to an indirect include of mpi.h in electrokinetics_cuda.cu, which is not permissible.

@RudolfWeeber , can you please also have an extra-close look at the coupling logic in the inertialess tracer coupling -- I think I got it right, and the tests pass, but still...

@pkreissl pkreissl changed the title WIP: LB coupling for RegularDecomposition without ghost shifts. LB coupling for RegularDecomposition without ghost shifts. Apr 9, 2022
@pkreissl pkreissl marked this pull request as ready for review April 9, 2022 15:18
@jngrad jngrad requested review from jngrad and RudolfWeeber April 11, 2022 14:23
Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

From a C++ perspective, this looks good to me. I ran the lb_momentum_conservation.py test on CPU with particle coupling, kT=0, 500 time steps, random initial positions and velocities for 20 particles on MPI topologies [1 1 1], [2 1 1], [3 1 1], [2 2 1], [1 2 2], [4 1 1]. The particle forces at the end of the simulations were identical within 7 decimal places, and were also identical to the same simulation on 1 MPI node with the N-square cell system.

RudolfWeeber
RudolfWeeber previously approved these changes Apr 19, 2022
@pkreissl
Copy link
Contributor Author

I think @christophlohrmann also wanted to have a look at this PR. What is the status?

@pkreissl
Copy link
Contributor Author

I just also ran the following tests on 1, 2, 3, 4, and 6 cores, respectively, to run them with different MPI topologies:

  • engine_lb.py as additional test for active particles.
  • virtual_sites_tracers.py; for that, I had to adapt virtual_sites_tracers_common.py, i.e., I used box_l = [6, 6, 12] and increased the number of integration steps for establishing the steady state flow field in the advection test (line 81) to 800.

All these additional test passed.

@jngrad jngrad added the Core label Apr 20, 2022
@jngrad jngrad added BugFix automerge Merge with kodiak labels Apr 20, 2022
@jngrad jngrad added this to the Espresso 4.2 milestone Apr 20, 2022
@kodiakhq kodiakhq bot merged commit 6f82ef2 into espressomd:python Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support LB coupling for RegularDecomposition without ghost shifts non_bonded_loop_trace seems to be broken
3 participants