Refactor reaction methods

[pm-blanco]

Fixes minor issues of #4251
Today we have been working with @davidbbeyer on some of the minor issues of #4251.

Description of changes:

• ReactionAlgorithm::make_reaction_attempt() now returns vectors instead of relying on output parameters.
• ReactionAlgorithm::generic_oneway_reaction() takes now as an argument a SingleReaction variable. We have done the same change in ReactionAlgorithm::save_old_particle_numbers, ReactionAlgorithm::all_reactant_particles_exist() and WidomInsertion::measure_excess_chemical_potential().
• We improved separation of concerns for particle creation/deletion/move in ReactionAlgorithm and reduced code duplication.

[jngrad]

@pm-blanco Could you please resolve the merge conflicts?

[pm-blanco]

@jngrad done.

[jngrad]

Thanks @davidbbeyer and @pm-blanco!

[jonaslandsgesell]

@jngrad , @KaiSzuttor , I found an interesting comment on whether to use custom structs or tuples https://google.github.io/styleguide/cppguide.html#Structs_vs._Tuples linked in https://abseil.io/tips/176
Would it have been better to go for the solution implemented here: 7b5bb27 ?

[jngrad]

The drawback of auto as a return type of a non-inline method is that the return type is only known in the translation unit where the method is defined. If you try to call the method from a child class, you will get

struct A {
  auto foo();
};
struct B: public A {
  auto bar() { return foo(); }
};

$> g++ -std=c++14 -c src.hpp
src.hpp: In member function ‘auto B::bar()’:
src.hpp:5:27: error: use of ‘auto A::foo()’ before deduction of ‘auto’
   auto bar() { return foo(); };

This means you will need to bring the struct declaration from the method scope to the class scope as soon as you find yourself in a situation where you need to call the method from outside the translation unit where it is defined, for example when writing a unit test.

I'm not totally convinced wrapping the return values in a struct to provide a namespace is helpful either. It also locks us with the names in the struct: if we later decide to rename any of the struct members, we have to update all call sites.

Personally, I find structured bindings sufficiently expressive:

#include<tuple>
#include<vector>
#include<iostream>

std::tuple<std::vector<int>, std::vector<float>, std::vector<double>> foo();

int main() {
  auto [p_ids_created, hidden_particles, changed_particles] = foo(); // C++17 feature
  std::cout << p_ids_created[0] << " "
            << hidden_particles[0] << " "
            << changed_particles[0] << "\n";
}

In this PR the std::tie(p_ids_created, hidden_particles, changed_particles) = foo() construction had to be used, because we're stuck with C++14 for hardware reasons. This has the unfortunate consequence that we need to spell out the types of the bound variables before the call to std::tie().

In another place in the PR, you see the following construction:

#include<array>
#include<vector>
#include<utility>

using Vector3d = std::array<double, 3>;

void place_particle(int pid, Vector3d const &new_pos);

void generate_new_particle_positions(int n_part) {
  using ParticleProperties = std::pair<int, Vector3d>;
  std::vector<ParticleProperties> cache;
  cache.reserve(n_part);
  for (int i = 0; i < n_part; ++pid) {
    auto const pid = i_random(/*...*/);
    auto const new_pos = /*...*/;
    cache.emplace_back(ParticleProperties{pid, new_pos});
  }
  /* ... */
  // restore original particle positions
  for (auto const & item : cache)
    place_particle(std::get<0>(item), std::get<1>(item));
}

Of course, I meant to forward the pair directly to the place_particle() function, but it didn't compile with C++14:

  // restore original particle positions
  for (auto const &item : cache)
    std::apply(place_particle, item); // C++17 feature

[jonaslandsgesell]

Thanks for your thoughts! I will think about it a bit more :)