Refactoring of parameter management

[michelebucelli]

This issue is a follow-up to multiple comment threads in recent PRs about the implementation of parameters parsing from XML files. It tries to summarize my thoughts about a possible refactoring, with the aim of serving as a starting point for discussion, eventually leading up to a concrete refactoring plan.

1. Definitions

In the interest of clarity, I'll define some terminology that I'll use throughout.

1. Parameter consumer: a module or class that is configurable through XML parameters. Currently, TTP06 is one example of consumer.
2. Parameter manager: a module or class that deals with reading the XML file and storing its content, which will be later transferred to a consumer. Currently, most of the classes defined in Parameters.h, deriving from ParameterLists, are managers.
3. Parameter schema: a description of what parameters a consumer needs (or a manager stores), in terms of their names, types (double, string, ...) and organization (XML node vs. attribute, nesting of nodes, ...). Currently, the schema is mostly stored in the managers.

2. Issues with current implementation

These are the main issues that I see with the current state of things, in descending order of how critical I think they are for extensibility and maintainability.

1. Information duplication. Every consumer must be shadowed by a manager. The manager's members, and the schema that they represent, depend heavily on the data organization of the consumer (in many cases, they're matchng 1:1). Therefore, changes in consumers must propagate to the managers, widening the blast radius of any change in terms of number of both classes and files. Similarly, the schema is stored in the individual managers, but it is duplicated in ParameterLists::params_map.

2. Mixed concerns. The managers deal with both parsing XML files and storing the data needed by the consumers. The concerns of parsing XML and doing something with the parsed data are only partially separated (ParameterLists goes in this direction, but managers derived from it still use tinyxml extensively). As pointed out by @ktbolt, this means that changes in the parameter file format have a very large blast radius.

3. Code duplication. Within Parameters.h and Parameters.cpp there is substantial code duplication. While ParameterLists tries to reduce this, its use is inconsistent (I have used it inconsistently myself).

3. Proposed redesign

One possible way to address these issues is described below. The details are tentative at best, and there likely current use cases that I haven't thought of (for some that I have thought of but haven't figured out, see below).

The design I'm aiming for here would allow for something like this:

class SomeConsumer : public ParameterConsumer {
public:

  virtual
  void declare_parameters() override {
    declare_parameter<double>("some_number", 1.0,   /* required = */ true);
    declare_parameter<bool>("some_flag",     false, /* required = */ true);

    declare_child("Nested_consumer", nested_consumer);
  }

  virtual
  void read_parameters() override {
    some_number = get_parameter<double>("some_number");
    some_flag   = get_parameter<bool>("some_flag");

    nested_consumer.read_parameters();
  }

protected:
  double some_number;
  bool   some_flag;

  SomeOtherConsumer nested_consumer;
};

3.1. Moving the schema declaration into the consumer

My largest concern is with the information duplication issue. I propose to address it by making sure that the schema is tied to the consumer, not the manager, and the manager becomes fully transparent to the schema.

To this end, I suggest to introduce an abstract class ParameterConsumer, to represent the consumer concept. This class is inspired by ParameterLists, except for the notable difference that consumers, and not managers, inherit from it. It offers general-purpose infrastructure for defining a parameter schema, and would (hopefully) be the only parameter manager class.

One way this could look is:

1. store a parameter map (or other container) inside ParameterConsumer. Unlike ParameterLists, this would store the actual instances of the parameters (as opposed to pointers to them);

2. expose methods declare_parameter<type>(name, required, default, ...) to add new parameters to the map (similar to ParameterLists::set_parameter, except that the method allocates the parameter instead of taking it from outside, and the name highlights the declarative purpose, rather than assignment);

3. declare a method virtual void declare_parameters() = 0 which derived classes override to declare all their parameters with declare_parameter<type>(...);

4. define a method read_xml(xml_element) for parsing the parameters from an XML element, after all parameters are declared, managing type validation, and populating the map with their values; this is similar in purpose to the various set_values that manager classes derived from ParameterLists currently have;

5. expose a method get_parameter<type>(name) to retrieve the value of the parameters after parsing;

6. declare a method virtual void read_parameters() = 0 which derived classes override to construct their state from the values from their parameters with get_parameter<type>(...).

3.2. Managing nested consumers

It often happens that a consumer contains another consumer, and this translates into nested XML elements in the parameter schema. I propose that the ParameterConsumer class also offer infrastructure to manage this in a structured way. One possible way is:

1. define a method ParameterConsumer::declare_child(const std::string &name, ParameterConsumer &child), by which the child is registered into the parent (i.e. stored in a container of children of some sort). This way, ParameterConsumer-derived classes effectively form a singly-linked tree structure;

2. the individual consumer classes are responsible for calling declare_child within their implementation of declare_parameters, and similarly for calling read_parameters on the children within their implementation of read_parameters;

3. ParameterConsumer::read_xml not only reads the parameter for the current instance, but also recurses into children. This way, it would be possible to read all parameters with a single call to e.g. Simulation::read_xml(root_node).

4. Known issues

1. Perhaps most important than all, this is a large refactoring with a massive blast radius. While I think the endpoint is desirable, it would be good to figure out a plan for gradual migration, especially since it is likely that many problems will become apparent during the implementation.

2. The above structure doesn't account for repeated tags with the same name, which are used very often (e.g. multiple Add_equation tags, or multiple Stimulus tags after Support multiple CEP stimuli per domain #586). One possible solution could be to have ParameterConsumer expose a dedicated interface for tags that are possibly repeated, but I haven't figured that out yet, and it might be a major blocker.

3. In some cases, the schema is attribute-dependent, e.g. <Viscosity type="Potential"> ...potential viscosity parameters... </Viscosity>. One way to address this is similar to what I have implemented for IonicModel and ActiveStress, but I'd have to understand this a bit more.

4. The above structure doesn't account for <Include_XML> tags. One relatively easy way to get it working would be to do it as a preprocessing step within ParameterConsumer::read_xml.

5. There still is some information duplication between the declare_parameters and read_parameters methods. I imagine that these methods would be always in the same file, and presumably in the same portion of the same file, which mitigates the duplication issue. If we wanted to remove it entirely, we could add an argument to ParameterConsumer::declare_parameter with a lambda function defining how to read the parameter. This way, all information about a parameter would be bundled in one place. There's downsides to this too, but we can discuss it further.

6. After reading, parameters need to be broadcast in parallel. This could either be done with a distribute method, to be called after read_parameters, and which would recurse into children. I think this can be kept independent of the parameter refactoring for the moment, seeing as distribute.cpp currently works downstream of parsing parameters and is somewhat independent of them.