WIP: Re-introducing three point coupling #2551

KaiSzuttor · 2019-03-04T11:13:00Z

Fixes #2517

Description of changes:

reimplementation of three-point interpolation
added tests

…e_point_coupling

…three_point_coupling

codecov · 2019-03-04T12:35:00Z

Codecov Report

Merging #2551 into python will increase coverage by <1%.
The diff coverage is 91%.

@@           Coverage Diff           @@
##           python   #2551    +/-   ##
=======================================
+ Coverage      75%     75%   +<1%     
=======================================
  Files         494     494            
  Lines       28095   28124    +29     
=======================================
+ Hits        21229   21252    +23     
- Misses       6866    6872     +6

Impacted Files	Coverage Δ
src/core/communication.cpp	`70% <ø> (ø)`	⬆️
src/core/grid_based_algorithms/lbgpu.hpp	`100% <ø> (ø)`	⬆️
...rc/core/grid_based_algorithms/lb_interpolation.cpp	`89% <87%> (-5%)`	⬇️
...ore/grid_based_algorithms/lb_particle_coupling.cpp	`90% <94%> (-2%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b453091...af1079e. Read the comment docs.

fweik · 2019-03-04T12:48:52Z

src/core/grid_based_algorithms/lb_interpolation.cpp

+
+void mpi_set_interpolation_order_slave(int order, int) {
+  if (order == 0) {
+    interpolation_order = InterpolationOrder::linear;


You can do interpolation_order = InterpolationOrder{order} without the switch, then you don't have to hard-code the values.

this is c++17

Ah yeah sorry, you have to do static_cast<InterpolationOrder>(order).

what about interpolation_order = static_cast<InterpolationOrder>(order);?

this is probably not such a good idea:
https://gcc.godbolt.org/z/Re_etN

That is fine here because the value is generated from an enum.

If you don't want that you can send the enum value via mpi, they can be serialized...

fweik · 2019-03-04T12:51:58Z

src/core/grid_based_algorithms/lb_interpolation.cpp

-namespace {
+void lb_lbinterpolation_set_interpolation_order(
+    InterpolationOrder const &interpolation_order) {
+  if (interpolation_order == InterpolationOrder::linear) {


I think it is cleaner to just do

switch(interpolation_order) { case InterpolationOrder::linear: case InterpolationOrder::quadratic: mpi_call(interpolation_order_slave, static_cast<int>(interpolation_order)); mpi_set_interpolation_order_slave(static_cast<int>(interpolation_order), 0); }

or skip the switch if the value checking is not needed. This way you can avoid hard-coding the values, and a compiler warning is emitted if one of the cases is not handled.

fweik · 2019-03-04T12:52:42Z

src/core/grid_based_algorithms/lb_interpolation.cpp

@@ -72,12 +100,21 @@ lb_lbinterpolation_get_interpolated_velocity_global(const Vector3d &pos) {
  if (lattice_switch & LATTICE_LB_GPU) {
 #ifdef LB_GPU
    Vector3d interpolated_u{};
-    lb_get_interpolated_velocity_gpu(folded_pos.data(), interpolated_u.data(),
-                                     1);
+    if (interpolation_order == InterpolationOrder::linear) {


switch(interpolation_order)

fweik · 2019-03-04T12:52:57Z

src/core/grid_based_algorithms/lb_interpolation.cpp

    return interpolated_u;
 #endif
  } else if (lattice_switch & LATTICE_LB) {
 #ifdef LB
+    if (interpolation_order == InterpolationOrder::quadratic) {


switch(interpolation_order)

fweik · 2019-03-04T12:53:33Z

src/core/grid_based_algorithms/lb_interpolation.cpp

@@ -92,6 +129,10 @@ lb_lbinterpolation_get_interpolated_velocity_global(const Vector3d &pos) {
 #ifdef LB
 void lb_lbinterpolation_add_force_density(const Vector3d &pos,
                                          const Vector3d &force_density) {
+  if (interpolation_order == InterpolationOrder::quadratic) {


switch(interpolation_order)

fweik · 2019-03-04T12:54:12Z

src/core/grid_based_algorithms/lb_particle_coupling.cpp

-      lb_calc_particle_lattice_ia_gpu(couple_virtual,
-                                      lb_lbcoupling_get_gamma());
+    if (lb_particle_coupling.couple_to_md && this_node == 0) {
+      if (lb_lbinterpolation_get_interpolation_order() ==


switch(interpolation_order)

fweik · 2019-03-04T12:55:33Z

src/python/espressomd/lb.pyx

+            Parameters
+            ----------
+            interpolation_order : :obj:`int`
+                0 refers to linear interpolation, 1 to quadratic interpolation.


Why not directly use strings for the interpolation order?

fweik · 2019-03-04T12:59:16Z

src/core/grid_based_algorithms/lbgpu_cuda.cu

-      (delta[7] * delta_j[2]));
+                                LB_node_force_density_gpu node_f,
+                                bool three_point_coupling) {
+  int max_node = 8;


This information should be encoded into the type of node_index and delta, this is very brittle...

fweik · 2019-03-04T13:01:20Z

src/core/grid_based_algorithms/lbgpu_cuda.cu

@@ -2307,12 +2246,13 @@ __global__ void
 calc_fluid_particle_ia(LB_nodes_gpu n_a, CUDA_particle_data *particle_data,
                       float *particle_force, LB_node_force_density_gpu node_f,
                       LB_rho_v_gpu *d_v, bool couple_virtual,
-                       uint64_t philox_counter, float friction) {
+                       uint64_t philox_counter, float friction,
+                       float *lb_boundary_velocity, bool three_point_coupling) {


Since there are only two coupling schemes, and this code is performance relevant, I think the coupling scheme should be selected statically and code for both schemes without the branches should be generated. E.g. make the coupling scheme a template parameter and instantiate both versions of this kernel.

…three_point_coupling

KaiSzuttor · 2019-03-06T13:36:18Z

@fweik

fweik · 2019-03-06T13:49:17Z

src/core/grid_based_algorithms/lb_interpolation.cpp

+void lb_lbinterpolation_set_interpolation_order(
+    InterpolationOrder const &order) {
+  interpolation_order = order;
+  boost::mpi::broadcast(comm_cart, interpolation_order, 0);


The mpi calls need to be the other way around, this is a deadlock.

Could you please explain?

You are blocking on boost::mpi::broadcast before the callback is invoked.

RudolfWeeber · 2019-03-06T17:37:07Z

I think, this pr was merged prematurely, I'm afraid.

This PR decreased the performance of the 2pt coupling by a factor of 3,
I now have a time step of 1.2 sec instead of 0.4 sec for 500k particles.

If the reason for the drag cannot be removed, the changes to the 2pt coupling need to be reverted.

KaiSzuttor added 18 commits February 28, 2019 12:07

LB GPU: index calculation cleanup.

a9c47ba

LB GPU: remove unused output paramter.

774a76b

LB GPU: cleanup neighbor index calculation.

0522c9c

LB GPU: only calculate necessary modes in interpolation kernel.

60bca58

Formatting.

539c4c5

LB GPU: index in mode calculation shortcut.

07940c7

LB_GPU: started to implement three-point coupling.

49a7b51

LB_GPU: correct the node_indices for three-point coupling.

509840b

LB_GPU: first non working version.

c1e1a71

LB GPU: first working version of three point interpolation.

b01fdae

Merge branch 'lb_cleanup' of github.com:KaiSzuttor/espresso into thre…

271ad85

…e_point_coupling

LB: three point for particle coupling.

afc816e

Merge branch 'lb_cleanup' of github.com:KaiSzuttor/espresso into thre…

f32122b

…e_point_coupling

Merge branch 'python' of https://github.com/espressomd/espresso into …

74e2720

…three_point_coupling

LB interpolation: added test and interface for interpolation.

cd20f00

LB interpolation: added test for particle coupling.

7e6e3cc

Formatting.

7fca5d3

LB: added missing include.

58efcef

KaiSzuttor changed the title ~~Re-introducing three point coupling~~ WIP: Re-introducing three point coupling Mar 4, 2019

KaiSzuttor added 3 commits March 4, 2019 13:18

LB: deactivate three-point coupling for test.

9baaf2e

LB: repair build without LB feature.

472ef2e

LB: rename interpolation functor.

924bc13

fweik suggested changes Mar 4, 2019

View reviewed changes

KaiSzuttor added 6 commits March 4, 2019 17:33

LB: use switch statements.

136370e

LB: use tag dispatch.

9bab627

LB interface: workaround cython limitations.

f0b198c

Merge branch 'python' of https://github.com/espressomd/espresso into …

a824d4c

…three_point_coupling

LB: documentation.

3eabf93

Formatting.

ebb50c6

fweik suggested changes Mar 6, 2019

View reviewed changes

LB: exchange call order in core interface.

af1079e

fweik approved these changes Mar 6, 2019

View reviewed changes

fweik merged commit b24864f into espressomd:python Mar 6, 2019

KaiSzuttor deleted the three_point_coupling branch March 6, 2019 15:54

jngrad mentioned this pull request Jul 8, 2019

engine_lbgpu broken in debug build #1575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Re-introducing three point coupling #2551

WIP: Re-introducing three point coupling #2551

KaiSzuttor commented Mar 4, 2019

codecov bot commented Mar 4, 2019 •

edited

Loading

fweik Mar 4, 2019

KaiSzuttor Mar 4, 2019

fweik Mar 4, 2019

KaiSzuttor Mar 4, 2019

KaiSzuttor Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

fweik Mar 4, 2019

KaiSzuttor commented Mar 6, 2019

fweik Mar 6, 2019

KaiSzuttor Mar 6, 2019

fweik Mar 6, 2019 •

edited

Loading

RudolfWeeber commented Mar 6, 2019

WIP: Re-introducing three point coupling #2551

WIP: Re-introducing three point coupling #2551

Conversation

KaiSzuttor commented Mar 4, 2019

codecov bot commented Mar 4, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaiSzuttor commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fweik Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

RudolfWeeber commented Mar 6, 2019

codecov bot commented Mar 4, 2019 •

edited

Loading

fweik Mar 6, 2019 •

edited

Loading