Decouple factory-creation from base classes

[nilsvu]

@wthrowe Do you think it’s conceptionally possible to do factory-creation from a base class without templating the derived classes on registrars? I’m asking because the Registrars template parameter makes it quite a bit more difficult to move code into cpp files. I’m working around that in current code (see e.g. https://github.com/sxs-collaboration/spectre/blob/develop/src/Elliptic/Systems/Elasticity/BoundaryConditions/LaserBeam.hpp), but perhaps there’s a better way. Here's a sketch of the current pattern:

template <typename Registrars>
class Base {
  using creatable_classes = Registration::registrants<registrars>;
};

template <typename Registrars>
class Derived;

namespace Registrars {
struct Derived {
  template <typename Registrars>
  using f = ::Derived<Registrars>;
};
}  // namespace Registrars

class DerivedImpl {
  using options = tmpl::list<>;
  DerivedImpl() = default;
  // This function can be defined in a cpp file
  void do_something();
};

template <typename Registrars = tmpl::list<::Registrars::Derived>>
class Derived : public Base<Registrars>, public DerivedImpl {
  using DerivedImpl::DerivedImpl;
  // pup, get_clone etc
};

struct OptionTag {
  using type = std::unique_ptr<Base>;
  static constexpr Options::String help = "A derived class of your choice";
};

This is fairly complicated and could be a bit of an obstacle for new people learning the code, since it's actually reasonably likely that some of the first code that new people come in contact with follow some variation of this pattern (e.g. analytic solutions / data, boundary conditions, events and triggers, equations of state).

Now typically the derived classes don’t actually need to know anything about their siblings, and the need for the Registrars template parameter only comes in because the base class has to provide the list of creatable_classes to the options code. So perhaps we could decouple this and, for example, have the option tag provide the creatable_classes instead. This would mean the base class and the derived classes don't need to keep track of registrars. I'm thinking something like this:

class Base {
  // Just a base class doing base class things
};

class Derived : public Base {
  // No `Registrars` template parameter, so things are easier to implement
  using options = tmpl::list<>;
  Derived() = default;
  // This function can be defined in a cpp file
  void do_something();
  // pup, get_clone etc
};

struct OptionTag {
  using type = std::unique_ptr<Base>;
  // We specify the possible choices of derived classes here (for example)
  using creatable_classes = tmpl::list<Derived>;
  static constexpr Options::String help = "A derived class of your choice";
};

Now of course the code that works with the factory-created object has to deal with the fact that the Base doesn't know its derived classes anymore, so in case it wants to use the "FakeVirtual" mechanism it has to keep track of the list of derived classes itself. I feel like this could be a worthy tradeoff. What do you think, could something like this work?

[wthrowe]

It would be nice to be able to move this stuff into cpp files.

I think we have enough control of the parsing process that we could get additional (compile-time) information into the factory parsing function, so it's not out of the question to pass the list outside of the class itself. Passing a list of derived classes isn't sufficient, as we can parse more than one base class at a time (e.g., std::unordered_map<std::unique_ptr<Trigger>, std::vector<std::unique_ptr<Event>>>), but we could try a compile time map from base class to derived class list or something.

[wthrowe]

We would also need to handle registering classes with charm somehow.

[nilsvu]

Yea good point. Could probably be done by just using the register_classes_in_list function (or whatever it‘s called).

[wthrowe]

Yeah, I guess this wouldn't really make registration any harder than it is now.

[wthrowe]

The best idea I've had is to put a map base -> derived list in the metavariables, which the factory functions already have access to (at least usually). This also puts all of the information needed for charm registration in one structure, which could simplify the executable headers a little.

The problem with this is that we only get a single global list for each base class, and we want different lists for different purposes. For example, under LTS, classes derived from StepChooser select both the step and slab sizes, and some classes only make sense for one of those uses.

[nilsvu]

Could we place such a map (or a metafunction creatable_classes<Base>, though a map might be easier to implement), in the option tag instead of the metavariables, so things like the StepChooser can specify the list of derived classes more locally? We could also/alternatively retain the ability that the base class specifies its creatable_classes for backwards compatibility.

[nilsdeppe]

Just some quick thoughts (I don't fully understand the threading of the discussion, so I'm just commenting here):

• having a tmpl::map between base classes and derived classes seems reasonable.
• keeping that in the Metavariables has the advantage of reducing places that need to synchronize the base and derived classes, at the tradeoff of a larger Metavariables. This map being in the option tag is also reasonable, but will make automatic registration more difficult.
• in the StepChooser case, could the base (and derived) classes be templated on a bool/enum that distinguishes Slab and Step choosing? We can create instantiations for the classes in cpp files fairly easily when we can enumerate the template parameter options. This gives the "different base classes" needed, but still allowing for moving code into cpp files when possible. If the interface between the choosers is uniform, a two-level hierarchy could be used as well, Base<SlabOrstep> :public GenericBase, and then the Base<SlabOrStep> could be used for registration, etc., but some generic functions might be able to take std::unique_ptr<GenericBase>. We could even have the create_from_options function convert the pointers of Base -> GenericBase.
• if we have a single global list, registration becomes very easy, I think. We can have a register_with_charm function that takes the tmpl::map, and then does all the loops and all the registration.

[nilsvu]

BTW here's another artifact of the current design that could be mitigated by moving the creatable_classes outside the base class: When I edit the header of one of the domain creators, then all domain creators recompile because they are coupled through the DomainCreator.hpp header.

[nilsdeppe]

This is avoided in the boundary corrections and boundary conditions by having a Factory.hpp that collects the list of header files, rather than doing it in the base class header. This means you can only use factory creation when you include the appropriate Factory.hpp, but that seems reasonable. There is still the issue that all the tests that test factory creation need to be rebuilt, but I don't really see a great way around that, though it might be possible with the proposed design. The tradeoff/question there would be if we think testing factory creation with only the one class being tested in the creatable_classes list is sufficient (probably, since we aren't testing the factory function itself, but rather that the class can be parsed by the factory). Of course, the Factory.hpp doesn't help if every derived class depends on the base class through the registrars

[wthrowe]

So, here's a possible plan, based on the above discussion:

• Constructible classes are defined in a tmpl::map of base -> tmpl::list<derived> in the metavariables.
• creatable_classes and registrars are gone (factories already get the metavariables if they are passed to the option parser).
• A given base class can only have one list. If multiple lists are needed the base class can have a dummy template parameter to distinguish them. This will still allow derived classes to be defined in cpp files using explicit instantiations.
• Charm registration becomes easy (from the consumer side): a single function can process the entire map.
• Methods using call_with_dynamic_type take the metavariables as a template argument.
• Methods using fake-virtual have to be rewritten in terms of call_with_dynamic_type.
• Sections of the code can define files like the Factory.hpp used by the newer boundary stuff that include headers if they wish. Such files could also define pre-made lists of derived classes for use in the map in the metavariables.

[nilsvu]

Sounds like it could work 👍 A few notes:

• We don't have to pass down the whole Metavariables for call_with_dynamic_type, the list of derived classes is enough. For code that uses a Factory.hpp file to define the list, we don't have to pass down anything at all.
• I think we can make the change gradually by constructing the map in the metavars from the Base::creatable_classes for now, and then convert the base classes one-by-one.
• Re base class with multiple lists of derived classes, e.g. StepChooser: The template parameter works. We could also allow the Step and Slab option tags to override the map of creatable classes. That shouldn't affect registration, since the metavars already registers everything.

[nilsdeppe]

There is something that would break with moving the classes to the option tag: the loops I do over the derived classes for boundary corrections and boundary conditions. I think it would be rather tedious/difficult/annoying to get the derived classes out of the options tag deep inside actions, but I haven't thought about this in detail. Getting the list out of the metavariables would be much easier since we actually have the metavariables passed around.

[wthrowe]

• We don't have to pass down the whole Metavariables for call_with_dynamic_type, the list of derived classes is enough. For code that uses a Factory.hpp file to define the list, we don't have to pass down anything at all.

Something has to associate the type list in Factory.hpp with the base class.

True that we don't need the full metavariables for cwdt, though. It may be cleaner to have the caller extract the relevant information.

• I think we can make the change gradually by constructing the map in the metavars from the Base::creatable_classes for now, and then convert the base classes one-by-one.

True.

• Re base class with multiple lists of derived classes, e.g. StepChooser: The template parameter works. We could also allow the Step and Slab option tags to override the map of creatable classes. That shouldn't affect registration, since the metavars already registers everything.

It seems like this gets tricky when you have classes that can factory-create themselves (possibly indirectly). To put the list in a tag, the class containing the option tag has to be templated on it, but you can't pass a class as its own template argument.

Edit: Deleted duplicate posts. Thanks GitHub.