Register initial magnetic field in EvolveGhValenciaDivClean with Charm.

[ncorsobh]

Proposed changes

Registers InitialMagneticField with Charm in EvolveGhValenciaDivClean. Necessary for evolving a magnetized TOV star with GH.

Upgrade instructions

Code review checklist

• The code is documented and the documentation renders correctly. Run
  make doc to generate the documentation locally into BUILD_DIR/docs/html.
  Then open index.html.
• The code follows the stylistic and code quality guidelines listed in the
  code review guide.
• The PR lists upgrade instructions and is labeled bugfix or
  new feature if appropriate.

Further comments

[wthrowe]

Instead of doing this, switch the class to use the factory_creation struct in the metavariables instead of the deprecated creatable_classes feature. Look at how it's done for TimeStepper, for example.

[nilsdeppe]

This was intentionally not using the factory creation. I think it should stay as-is. The factory creation struct is tedious when you know all the derived classes ahead of time. We've discussed that the creatable classes is not deprecated a long time ago and serves a distinct purpose from the factory class approach. There's a lot of code that intentionally doesn't use that and I think should stay that way.

[nilsdeppe]

Looks great! Thanks for doing this!

[wthrowe]

This was intentionally not using the factory creation. I think it should stay as-is. The factory creation struct is tedious when you know all the derived classes ahead of time. We've discussed that the creatable classes is not deprecated a long time ago and serves a distinct purpose from the factory class approach. There's a lot of code that intentionally doesn't use that and I think should stay that way.

Who's "we"? I don't recall being involved in any such conversation. The code clearly marks it as a legacy interface.

The factory_creation method requires, depending on how you count, either less code or about the same as creatable_classes, so I don't see how it's more tedious.

[nilsdeppe]

This was discussed in either the Thursday or Monday call maybe 3-4 years ago. Definitely quite a while ago and you were present. My objection to removing the using createable_classes is that there are places in the code where we can currently do explicit instantiations because we know all the derived classes without the metavariables. Requiring the metavariables means more code can only get compiled as part of the executable. I'm fine with using the factory struct being used as tmpl::pair<Base, typename Base::createable_classes> but I don't like forcing everything to have typelists outside of the base class. That is useful in some cases where we want different derived classes depending on the executable (e.g. it's fantastic for initial data and events), but things like boundary corrections and boundary conditions are quite universal. Being able to have a call_with_dynamic_type over those in an explicit instantiation in the System directory is very nice. Keeping the registration with Charm++ through the factory_creation struct seems good to me, though.

So if your suggestion is change the factory_creation struct in the metavariables to have tmpl::pair<Base, typename Base::createable_classes> then I'm in favor. If the suggestion is to move the createable_classes typelist into a separate file, then I'm opposed, certainly in the general case, but currently also in the specific case.

[wthrowe]

(Your time estimates are presumably off. This code was only added a bit over 3 years ago.)

You can't explicitly instantiate a factory, and that's the only thing that is directly tied to the factory_creation struct. The argument to call_with_dynamic_type doesn't have to come from the factory_creation struct if you have a list by some other means, so in those cases having the list available from the base class may make sense, yes.

In any case, that's irrelevant for this PR, since there's no reason to use call_with_dynamic_type with InitialMagneticField, and nothing is doing anything fancy with option parsing. What's the advantage of having the list in the base class? You still need to have the list of headers in a Factory.hpp, so it means the same information has to be maintained in two places, and there's very little you can do with the list without including the class's Factory.hpp.

[nilsdeppe]

I talked about this with @wthrowe on Friday and the conclusion was that in this case moving the list of magnetic fields from the base class to Factory.hpp is fine. The plan is to make that the convention whenever possible. I've had trouble finding where the lists are located, particularly with initial data, so trying to standardize that (not in this PR!) would be useful.

[ncorsobh]

Force-pushed since the previous version was just a one-liner that got removed anyway. Let me know if this is the idea you're asking for or if I should do something else.

[wthrowe]

Yes, that's what I had in mind. Tests need fixing, though.

[wthrowe]

Remove the forward declarations from this file.

[wthrowe]

Don't change this so much. You've lost the test of the get_clone() method and possibly the option tag (not sure if that one's important here). Just switch test_option_tag_factory_creation to test_option_tag and pass the metavariables.

[wthrowe]

[optional] I think the only change you need in this file is making this line register_classes_with_charm<InitialMagneticFields::Poloidal>();. The version you've written isn't wrong, but it's a lot more verbose than necessary.

Same for Test_Toroidal.cpp.

[wthrowe]

OK, looks good. Squash.