Switch the consensus algorithms interface to using function objects.

[bangerth]

This is a proposal to fix #13208. The idea is to switch the CA run() function to function objects instead of the Process object. I've managed to keep the existing functions around, and the patch currently does not convert any of the existing CA users. In other words, things are backward compatible. I will change the users to the new interface in a later patch or commit.

Here is what I was setting out for:

• Ultimately, I would like to address Specialize consensus algorithms for the case where no answer is required #13084, where we need to figure out whether an answer is actually required (or would just be empty) and if not, would use a more efficient algorithm. But for this, I need an interface where one can indicate this, which is most easily done by calling a variant of the run() function that simply doesn't take answer_request() and process_answer() (renamed from read_answer()) functions.
• The interface essentially makes the AnonymousProcess class unnecessary: Instead of storing the lambda functions in AnonymousProcess, one can just pass them to run() directly. This is what I would have liked to do in FETools::extrapolate(), for example.
• Retain the possibility of using the Process class -- I keep a Interface::run() function that simply unpacks the Process object and calls the one that takes function objects.
• While there, I also changed the type of the function objects users have to provide. Previously, these were all of the form

  void create_request(const unsigned int other_rank, std::vector<T1> &  send_buffer)

and put their output into the last function argument. But in all cases, these output buffers are not reused, and so there is no good reason to not just write the function as

   std::vector<T1> create_request(const unsigned int other_rank)

where the object is simply returned. In times of move constructors, this should not have any adverse effect, but is more "functional" and easier to read. In some cases, it also avoids having to use named variables.

Overall, this patch should have no actual functional difference, and it should be backward compatible.

Thoughts about the approach?

/rebuild

[bangerth]

In a side-by-side, you would see that this resetting of the member variable happened in the constructor. I had to move this into the run() function because we now have two constructors, only one of which knows how to choose the algorithm. That also means that the consensus_algorithm variable is actually only used in this one run() function any more. I could move it from a member variable to a local variable. I'll do so in a follow-up patch, but thought I'd try to keep things minimal for this PR.

[bangerth]

This is strictly speaking an unrelated addition, but I thought it was a good idea while I was already working on this function.

[peterrum]

Looks overall good.

But to be honest, I am not that fond of that additional parameters need to be passed to the internal functions and it does not help the reading (the NBX algorithms used to be 7 locs). The logical next step would be to move all internal variables into the run function and at that time one could make the function static and probably one would not even need a class but only a free function. The helper functions would have even longer argument lists...

[bangerth]

Yes, it is not crazy to suggest that a free function would have been the way to go.

To me as a software designer, here is the difference in variables:

• Member variables describe the state of the object.
• Function arguments describe the inputs of the operation to be performed by a function.

This is the separation I have chosen in this patch: I pass the inputs around as function arguments, and describe the state of the object via member variables. I think I did that consistently.

Now, oftentimes, member variables would describe and keep the state of the object between one function call and the next. Here, this is not the case: At the end of the run() function, all state becomes irrelevant and it is created from scratch during the next call to run(). This might suggest that we implement (the public interface of) these algorithms not as a member function, but as a free function ConsensusAlgorithms::nbx(...) (and similarly for PEX), which would then simply create an object of a class declared in some anonymous namespace that keeps the state of the algorithm; when CA::nbx() terminates, it also destroys the object -- in other words, the object only exists only during the run time of run(...) and keeps the state of the object between the different phases of the algorithm. This is probably how I would have implemented the algorithm if I had to start from scratch, but it seemed like a step too far given the existing design. I'm willing to be convinced otherwise.

(By way of already arguing the opposite direction: One could of course say that the operations that are encoded in the Process object are also part of the state of object, and so it makes sense to store them in a member variable. This would, for example, make sense if one were to pass a Process object to the constructor, which would then do some setting up of communication data structures, and then repeated run() calls would use all of this. If so, storing the Process object as a member would make great sense. But this is neither what the implementation of the algorithms currently do, nor how they are used in practice: As far as I can see, there is no place in the library where we actually store a ConsensusAlgorithms object; to the best of my knowledge, they are always created in one place, then one calls run(), and then the object goes out of scope again. In other words, empirically these objects are treated like a free function -- in the sense that there is no conceptual difference between { Class c; c.run(); } or even Class().run() and function_name() -- , and in the places where the CA algorithms are currently use all seem to treat the Process objects as a convenient way to package up function arguments, rather than as setting the initial state of an object that will then be re-used many times over.)

All this to say that I don't think the current design of these classes is bad; I just wanted to provide a software-design-theoretic explanation for why the patch is written this way :-)

[bangerth]

Maybe to bring this into one sentence:

Function arguments describe what to do on the member variables of a class.

[kronbichler]

I think this is a reasonable step. I am not particularly happy with the increasing number of function arguments, given that the state is not passed along between the member functions. Also, I generally prefer functions with a named signature (like member functions rather than lambdas) as they are much easier to track down when you are in some lower-level tool as I keep using them, but it seems a problem not shared by many others. On the other hand, I feel that being able to eventually call ConsensusAlgorithms::nbx(...) would be great.

[bangerth]

I commit to making ConsensusAlgorithms::nbx() available as a follow-up.

[kronbichler]

@peterrum any final comments here? I think we should go on with this topic now.