Bugfix/femdgoperator parameters

enabled an automatic merge when the pipeline for 7e831718 succeeds

@andreas.dedner: Can you take a look here. The problem is, that the defaults do not work anymore. I'm not sure if it is a good idea to replace the default parameter list with and empty dictionary. The, on the C++ side on does not get the default ParameterContainer. Is that a problem?

Default function parameters with mutable objects are problematic. Run

def test(x,y,a={}):
    a[x] = y
    print(a)
test(1,2)
test(2,3)

The initialization of a only happens once - after that it is always the same dictionary used. The same would happen with parameters. It is initialized empty on the first call but in subsequent calls it's always the same. That is a mistake I made often at the beginning of the corepy project... One needs to use None as default. What exactly is the issue with the 'defaults' not working?

canceled the automatic merge

When femDGOperator is called with parameters=None then one gets the following error. I have tried to fix it, but from your comment above I get the fix is not correct.

Traceback (most recent call last): File "main.py", line 34, in run(Model, File "dune-fem-dg/build-cmake/python/dune/femdg/testing.py", line 79, in run operator = femDGOperator(Model, space, File "dune-fem-dg/build-cmake/python/dune/femdg/_operators.py", line 220, in femDGOperator parameters["femdg.limiter.admissiblefunctions"] = "default" TypeError: 'NoneType' object does not support item assignment

The problem must have been introduced in 30191ac9 where the parameters is used right at the top of the function. I think the way to fix it is to have the following right at the top of the function (before parameters is used the first time):

if parameters is None:
   parameters = {}

This is a Python feature with the mutable default parameters that always requires this fix (same with param=[] for example which needs to be replaced with param=None and then if param is None: param=[]).

I'm always on the lookout for old code with param=[] or param={}....

Ok, that makes sense. But my worries was more that

if parameters is None:
   parameters = {}

now introduces the empty parameter list as default parameter, when before it was Parameter = Dune::ParameterContainer or something like that.

So I wonder if that will change anything. Otherwise I will fix it like that.

Right: so what happes is the following, e.g., for scheme construction in dune-fem/dune/fempy/py/scheme.hh line 72. If a pybind11::dict is passed to the constructor this is converted into a PyParameter​ class defined in dune-fem/dune/fempy/parameter.hh. Actually it is a ParameterReader​ with a special lambda. If a parameter is requested and that parameter exists in the python dict that is used otherwise the call is passed down to static Fem::Parameter instance.

In addition to prefix is removed, that is why on the python side scheme​ parameters don't need to include the fem.solver prefix.

So (I think) passing in {} will reduce efficiency a bit (since the PyParameter wrapper is used which probably isn't cheap due to the Pybind11 stuff in it but otherwise it should behave like ParameterContainer. We could change the function pyParameter to check if the pybind11:dict is empty and return ParameterContainer in that case?

This has already been merged to master in a different branch.

closed