Feature/parameterizedobjectfactory

the idea is to have a generic factory which can be extended by the user at run time and maps a key (usually an std::string) to a constructor-call (usually taking a Dune::ParameterTree).

We want something like a plugin-system, to instantiate implementations of an abstract interface. Usually a factory can only create a hard-coded list of objects. The proposed ParameterizedObjectFactory different modules (or the user) can register implementations of the interface, without modifying the factory code.

Example:

using V = Dune::BlockVector<Dune::FieldVector<double,2>>;
using M = Dune::BCRSMatrix<Dune::FieldMatrix<double,2,2>>;

using PreconditionerFactory = 
  Dune::ParameterizedObjectFactory<Dune::Preconditioner<V,V>(const M&, const Dune::ParameterTree&)>;

PreconditionerFactory::define<Dune::SeqGS<M,V,V>>("Gauss-Seidel");

Added 1 commit:

• 37d17762 - [parameterizedobject] allow for non-const parameters

Added 1 commit:

• 92b7e366 - [parameterizedobject] use unique_ptr instead of shared_ptr

Added 1 commit:

• 47dfcb6a - [parameterizedobject,doc] fix typo

Added 1 commit:

• b5a2550a - [parameterizedobject,doc] fix typo

Added 1 commit:

• efff03af - [parameterizedobject] use default constructor of no parameters are given

Added feature label

Title changed from [WIP] Feature/parameterizedobjectfactory to Feature/parameterizedobjectfactory

The tag parameter seems like an ugly workaround to the shortcomings of introducing global state. IMHO it would be a much cleaner and simpler design to not make everything static and drop the tag.

@carsten.graeser, perhaps I don't understand you correctly, but everything is static. This is exactly the reason for the tag parameter. With this parameter I can distinguish different factories for the base class.

Imagine a user code where I want to specify dynamically a source-function and a Dirichlet-extension. Both are some typo of global function mapping from \Omega to R and thus both will end up in the same factory, as long as I don't separate them explicitly. I do this by the tag parameter. Do you have other suggestion?

Sure, we could also ignore this problem... the user should know which one is the correct on, on the other hand, if both are referenced by the name of the example?

An alternative might be to add some type of namespace to interface, similar to the hierarchy in the ParameterTree. We could then prefix the key, iff the key is a string. In this case I think we should hard-code the key to be a string.

As I said I'm asking why not just make things non-static. Then you could simply use different factory objects for different needs. If you absolutely want to have global ones, because you don't want to pass them around, you can still store your instances globally.

Also the tags only solve the issue partially, because one may want different factories, where they cannot be distinguished statically.

Added 1 commit:

• 5dabbf84 - [parameterizedobject] remove singleton object

@carsten.graeser thanks for the input. You are right. It is easy enough to create a singleton outside the factory. I updaten the branch accordingly.#

More comments? Otherwise I'll merge, as Linus wants to build upon this branch for his istl work.

Your approach does not compile with gcc-4.9 because it does not allow function types with abstract return type, i.e., Interface(Args...) is not allows. As a remedy one may pass the Interface and Args types seperately. Unfortunately one cannot have the default key type then unless one packs the Args... pack into a type like, e.g., TypeList in dune-function. However this would be even less elegant to specifying the key type explicitly.

Attached you can find a patch that splits the arguments and also removes the *Base class which is useless since the tag parameter is gone.0001-Avoid-function-type-with-abstract-return-type.patch

I furthermore have a proposal for a generalization of this class. I'll try if this can be implemented and upload a second feature branch.

I'm not totally happy with your proposed patch, as it forces me to specify the string explicitly. Usually it will be string and it would be much more convenient, if this type could have default value.

I'll look into the gcc-4.9 thing.

It works also in gcc-4.9 if we use a pointer in the signature:

    Dune::ParameterizedObjectFactory<InterfaceB*(int, std::string)> FactoryB;
                                              ^^^

instead of

    Dune::ParameterizedObjectFactory<InterfaceB(int, std::string)> FactoryB;

(and obviously the template specialization has to be adjusted.

I'm also not completely happy with this. Using a pointer type also came to my mind, but it seems to be a hack. In fact we never have something like a function returning an Interface or Interface*.

I just pushed a the branch feature/flexibleparameterizedobjectfactory (8e70c875). The main change is that the return type of create is no longer tied to unique_ptr but is just T. If you register an Impl this automatically selects a proper creator function. Depending on T this will call make_shared, make_unique, or a constructor. The impact on user code is that you must now explicitly use unique_ptr instead of Interface as template parameter.

This allows to use this class also with shared_ptr or types with value semantics. Furthermore the branch adds support for providing custom creators. This is can be used via lambdas that allow to adapt a non-fitting type or to return a pre-created object stored in a closure. See the test for examples.

As a side-effect we can use the signature notation and a default key type again.

Hi Carsten, I looked at your branch.

What I like is the possibility to use a different construction call, in case that the constructor does not patch the required interface.

I'm not sure we really want all this magic to have the different storage types. If you want a shared_ptr you can just assign the result to a shared_ptr, as it can be assigned from a unique_ptr. Immediately creating the object does not actually fit into the context, as we have virtual interfaces and they don't work via objects. Did you test this? I don't get how it should work. You will alway slice to the base class and loose the desired functionality. I'm sure I overlook a particular use-case, but currently I don't see it.

Regarding the syntax, I'm currently not sure what we want to have... I'm still thinking about the different possibilities. Using the

Hi Christian, I added an example with value semantics to the test in my branch (28a50ce2).

Assume that you want a factory to provide functions for rhs, dirichlet data, coefficients, etc. Then you don't have a polymorphic but a type erased interface class like std::function and the conversion does not slice but wrap the created objects.

I'm also not convinced about the magic when storing an object. We may leave writing the lambda for this special case to the user. The other two cases, shared_ptr and unique_ptr, are just there to keep the simple usage of your original proposal.

BTW: If we drop them, too, and let the user write the one-line lambda forwarding to make_unique, then the whole class is just a map<key,fn> where define(key,creator) forwards to operator[] and create(key,args) mimics [key](args) for the unfortunately non-existing const operator[]`.

My initial idea was to allow an easy handling of these factories. So I'm not in favor of removing support for unique_ptr. Regarding type-erasure objects, you are right. Storage wise they are something like a shared_ptr, but with different semantics.

Wouldn't it be better to keep object and ptr semantics separated and provide different implementations? They can both derive from the same base class, but explicitly inject different creators.

My main concern is that I don't want to write things like ParameterizedObjectFactory<shared_ptr(Dune::ParameterTree)>, but perhaps it is just that I have to get familiar with this style...

In my opinion ParameterizedObjectFactory<shared_ptr<T>(Args)> makes the purpose much more clear than ParameterizedPointerFactory<T*(Args)>.

I don't understand your concern regarding the ease of use. It seems some characters where lost. But: Despite the type-name this solution is used exactly as yours.

As you brought up the use-case of type-erasure objects I thought about how this relates to dune-functions and would like to do some experiments. I won't have much time this week, but I'll try to provide some updates asap.

OK, after a long time, I remembered our old discussion again

I did some further tests with modifications proposed by @carsten.graeser and agree that they are useful. While I still think it is not so nice that the variant with the abstract base class (which I had in mind as the default) needs to specify the unique_ptr explicitly, I agree that the benefits of the new interface are bigger than the cosmetic downsides.

In particular I tested the issue of using global factories via a singleton, which is the pattern I would like to use for solvers, as this will guarantee extensibility, and this works nice and we can easily hide unique_ptr from the interface using template aliases.

I'll merge with Carstens branch and update the merge request accordingly.

I merged with @carsten.graeser's branch. Further more I updated the tests and added a version which is close to what we would need in dune-istl. I'm now quite happy with the version and propose to merge as is.

Added 616 commits:

• 5dabbf84...1410d4a9 - 604 commits from branch master
• 1ef346ca - Avoid function type with abstract return type
• 00cd88b8 - Merge branch 'master' into feature/flexibleparameterizedobjectfactory
• b1d476d2 - Use template parameter as return type
• 01f8e9f4 - Adjust test to new implementation
• ca6a2f1f - Add support for custom creator functions.
• f836a761 - Add support for storing copies that are handed out on creation
• f449b735 - Test new features of ParametrizedObjectFactory
• 8e70c875 - Reintroduce function syntax
• 507cb65d - Remove unused template parameter
• 28a50ce2 - Update test of ParametrizedObjectFactory
• 316a604b - Merge remote-tracking branch 'origin/master' into feature/parameterizedobjectfactory
• 5654e8ac - [test] modified extended parameterizedfactorytest

Compare with previous version

Added 1 commit:

• 07a4dc1b - [bugfix] fix include name

Compare with previous version

@christi: Looks good to me. But I've a few small comments.

• IMO the name define(...) is not very instructive. register(...) would be better, but is not possible. Maybe registerFoo(...) for with some suitable value for Foo is the way to go.
• The two overloads for define(...) may become ambiguous, e.g. for the signature bool() when passing an std::function. But this is maybe restricted to rare corner cases. However this could simply be avoided by excluding the copy-value overload if the argument has proper function signature.

Added 4 commits:

• cfcde36b - [test] avoid unused variable warning
• 139910ee - [test] test ambiguities of define in ParameterizedObjectFactory
• 9c830cb8 - [doc,parameterizedobject] fix docstring for copy-constructor-define
• 446e35bb - [parameterizedobject] fix ambiguities of define

Compare with previous version

I improved the enable_if to avoid the ambiguities.

If the build succeeds, I'll merge.

Enabled an automatic merge when the build for 446e35bb succeeds

Status changed to merged

Mentioned in commit 20c53865

@christi Thanks for taking care!