Decide on design for factory functions #95

Closed pjknowles closed 2 years ago

In GitLab by @KnowlesPJ on Dec 20, 2020, 18:37

Setting up a solver now seems quite complicated, and requires the user to know something about details that he doesn't need to for any other purpose. https://gitlab.com/molpro/linearalgebra/-/blob/master/test/create_solver.cpp seems quite hard to me. Of course for expert use that's fine, but I can think of two possible simpler interfaces that might be enough for most uses:

1.

template <OptionsType, Rvector, Qvector, Pvector>
std::shared_ptr<IterativeSolver<Rvector, QVector, Pvector>> solver(OptionsType& options);
auto mysolver = molpro::linalg::itsolv::solver<myR, myQ, myP>(myOptions);

2.

template <OptionsType, Rvector, Qvector, Pvector>
std::shared_ptr<IterativeSolver<Rvector, QVector, Pvector>> solver(const std::string& algorithm);
auto mysolver = molpro::linalg::itsolv::solver<myR, myQ, myP>("BFGS");

(apologies for poor C++ construction).

can of course call 1. 1. should set up default handlers and whatever else is necessary.

Thoughts?

In GitLab by @KnowlesPJ on Dec 21, 2020, 07:33

I guess I had not considered the existing SolverFactory. fcec9ae289e3b0a59204a0d5e0ac6284434d485d already makes this a bit easier to use, by removing the need to specify handlers, for which, surely, the default will almost always be what's wanted.

In GitLab by @KnowlesPJ on Dec 21, 2020, 07:37

The remaining roles of test/create_solver.cpp seem to be to specify what solver by creating a temporary options object, and dealing with the polymorphism of the logger. The first can be dealt with by a factory function for each interface class that takes just the desired object type as an enum argument. (For the linear solvers, this is overkill, because there is only one implementation class to choose, but for optimisation it's needed, and of course a sensible default such as BFGS can be specified). The options can always be got from the class by the user later.

It seems to me to be over-engineering to pattern-match on the type of an options object to decide which kind of solver is needed - more straightforward to simply provide factories for each kind.

In GitLab by @mxs301 on Dec 21, 2020, 17:54

test/create_solver.cpp is not really how the intended use of the factory. It was there before the factory and I just used it as a test.

Yes, enum is also an option and instead of overloading we could have separate function for each solver type. I thought it is easier to think about options as carrying all of the options. Plus, enums and no overloading is too C-like, but that is just personal style.

In GitLab by @mxs301 on Dec 21, 2020, 21:03

Summary of the last meeting:

Enums are not needed and the current approach of using overloading is fine
Array handlers should be optional, most of the users will want the default handlers to be constructed
Logger is not part of the interface and is considered our internal tool. We do not need to expose it in the factory
[x] need to implement create(std::string method, std::map<std::string, std::string> options) so that method and options can be passed as string variables. This is necessary for the wrappers.
Factory methods do not need to be in a class and could be free functions.
- keeping them in a class allows one line explicit instantiation
- overloading is an overkill and we can deal with mechanism for adding a user defined solver later

In GitLab by @mxs301 on Dec 22, 2020, 02:34

@KnowlesPJ I've added factory that takes map of string. We can remove virtualization and make the functions static, or make them free functions. I would prefer static, so that explicit instantiation is one line, but I don't have a strong opinion on this. I'll live this up to you. Feel free to merge it into master when you are happy with it.

In GitLab by @KnowlesPJ on Dec 22, 2020, 07:39

I don't have a strong feeling about virtualisation either, beyond that my head would probably hurt too much to contemplate inheriting from this, but somebody else might be stronger. I personally think it's simpler to have the free function, but am ready to understand a bit more why that would be a significant restriction.

The new factory looks good. I would suggest for consideration

I don't quite see the point of the generic IterativeSolver factory. You will always know at compile time which of the four classes of problems you are solving. Why not instead make a separate factory function for each of them?
making options_map options optional, ie defaulting to {}
making method optional, meaning if not given that the default method will be adopted.
actually complexity could be reduced by just making method one of the possibilities to be specified in option_map options.
A convenience wrapper where a single string is specified, with comma-separated key=value papers which get parsed into an option_map. This is going to have to be written anyway for the fortran interface, so it might as well be made available.

Here are some implementation examples for this second style of factory:

auto solver = LinearEigensystemFactory<myR,myQ,myP>();
option_map options; options["method"]="BFGS"; options["max_q_size"]="6"; auto solver = OptimizeFactory<myR,myQ>(options);
auto solver = OptimizeFactory<myR,myQ>("method=BFGS,max_q_size=6");

Finally, if it's going to be a free function, Factory is probably not the right name. Factory would name the class well, but the name of the function should probably indicate what is returned: createOptimize? create_Optimize?

I can certainly manage to do all this, but thought it worth continuing the discussion in the clear.

In GitLab by @mxs301 on Dec 22, 2020, 08:23

"always" is up to us. Current interface allows for a single point where solve() is called. For example, HF procedure can use DIIS or BFGS because it only takes an Options object and calls generic create. Arguably, same can be achieved by requiring solver to be passed instead of Options. I am fine with either, we can remove the generic create().
This is possible if method is still the first argument.
In terms of options_map function, argument method specifies implementation and solver type. It would not be a good idea to return default eigensolver if they need an optimizer but made a typo in the input {"metho","L-BFGS"}.
Yes, good idea.
Yes, good idea.

I agree that there is little point for a class, currently create can be made static. The only benefit is to simplify explicit instantiation, but one line vs four lines is almost the same.

I think we are very close and the things left to decide are small details.

Whether to keep generic factory std::shared_ptr<IterativeSolver> create(const Options&) or always make a particular solver type.
Whether to use free functions or a class.
Whether to use overloading or separate names for each solver type. I like your second naming suggestion create_IterativeSolver(), create_LinearEigensystem() etc.

I'm happy with any choice.

In GitLab by @KnowlesPJ on Dec 22, 2020, 08:54

I'll try and put this all together today, and then we can see how we like it.

In GitLab by @KnowlesPJ on Dec 22, 2020, 11:06

My answers to the above questions are

The generic factory is fine in principal, but I still doubt that it's very useful, as you will know which of the four you are using at compile time. In principle one could have only the generic, but then you'd probably need to immediately do a dynamic_cast. I suggest we keep it simple, and also simplify things by just putting these factory functions in the appropriate header files, eg ILinearEigensystem.h. I'll do that, and leave your class-based things in place, whilst we decide.
Free functions seem simpler to me. Anyone wanting to extend could probably more simply replicate than inherit.
Go with separate. It's simpler - the user does not have to think about the hierarchy IterativeSolver->LinearEigensystem too much.

I have a further suggestion for consideration once the dust has settled, namely a refactoring of names. It's an implementation detail that we have the abstract class layer. Therefore the user should see class name LinearEigensystem not ILinearEigensystem, and the concrete classes can be renamed to anything we like as they're invisible.

In GitLab by @KnowlesPJ on Dec 22, 2020, 16:11

On second thoughts, the factory class makes it simpler to write the factory instantiation cpp.

In GitLab by @KnowlesPJ on Dec 23, 2020, 17:24

Implementation of the factory basically done.

My conclusion after the extreme pain of this is that it really is too complicated. The trouble with having parallel SubspaceSolverOptBFGS and OptimizeBFGSOptions is that there is no relationship between them, and so if you want to move from one to the other, you have to iterate the possibilities BFGS, SD,... . This is all just a waste of effort and introduces lots of stuff the user has to know. It's also not quite true that only the subspace solver has to be different - in BFGS the solver itself has special code. All this can be discovered, but with work. Even more trouble if you want to get at the logger, as it is not a member of the interface class, so you have to do a dynamic cast.

I think it should just be done more simply. One implementation for each of the four LinearEigensystem, LinearEquations, NonLinearEquations, Optimize, just as it is now for the first two. The second two can then carry a class variable (probably enum) that decides on the method, and this be propagated to the subspace solver class on construction. Actually the different methods are almost the same anyway in their basic algorithm. The price is that the different methods have to share the list of options. I cannot see any other disadvantage.

Let's merge to master what we have now, and pick this up as a separate piece of work.

In GitLab by @KnowlesPJ on Dec 23, 2020, 17:34

I furthermore think, after reflection, that it doesn't really help very much to separate off the "method" option when options are given as character strings. If someone comes from fortran with "metho=bfgs,..." it's not really clear what you can do to help, nor is it completely clear to me why this is worse than any other typo. One could take the stance that the method must be provided always, but the counterview is that there's nothing wrong with a default. It would be simpler and more logical to carry all the options around in just one struct.

In GitLab by @KnowlesPJ on Dec 23, 2020, 17:42

A further simplication would then be to get rid of the ILinearEquationsOptions etc layers. What is the point of this, when only LinearEquationsOptions inherits from it, and that class has a richer public interface (the actual options as public class variables)?

Of course ILinearEquations has to stay, because LinearEquations inherits from it via SolverTemplate. That's completely different.

In GitLab by @mxs301 on Dec 23, 2020, 17:58

The relationship is through BFGS class which is the implementation of IOptimize. I'm not sure why one needs to iterate over possibilities. The decision of whether to have separate implementations or merge them into one class is up to the developer. It's not a mistake to implement Optimize and adding method="BFGS" or method="SD" option, if they are logically and structurally very similar. Same as the is traditional LinearEquaitons solver and augmented Hessian.

However, I wouldn't abandon the mechanism for separating implementations. We might want to implement a completely different type of solver, and still be confident that the old code works.

The Logger is our implementation detail and the user won't need to get at it.

Again, I'd rather stick with the SOLID principles and the O is telling me that we need a mechanism for separating the implementation.

In GitLab by @mxs301 on Dec 23, 2020, 18:00

Other typos will have sensible defaults. "BFGS" and "SD" are quite different things, and instead of realising that the user made a typo they might just think that the solver is rubbish and convergence is super slow.

But, I don't feel very strongly about it, so whatever you think is best.

In GitLab by @mxs301 on Dec 23, 2020, 18:05

This is needed if you want the generic create that returns IterativeSolver*

molpro / iterative-solver

Decide on design for factory functions #95