The global parameter "allow_extrapolation":
Must be set prior to the creation of a Function to have effect. The error message does not says this.
Should not be a global parameter. It should be set per Function.
I don't know how to fix this and still use the parameter system.
This is not a priority for me, so if this has major priority please reassign.
How about this: at the point when this is first used in the code, we do this:
if (!_allow_extrapolation.second)
_allow_extrapolation = std::make_pair<bool, bool>(parameters["allow_extrapolation"], true));
Then make that parameter local to the function and add a member variable
std::pair<bool, bool> _allow_extrapolation;
The first item in the pair is the parameter value and the second is whether we have read it from the parameter system.
I would prefer to just add a member function to Function that take an enum to turn extrapolation on/off. The default value could be taken from the parameter system.
I don't like sneaking in lots of bool options and member variable flags in place of using the parameter system - unless we can find a consistent way of doing it. I'm not particularly advocating the Parameter system for this purpose, I'm just saying it needs to be consistent. The parameter system was an attempt to solve this issue and make it uniform. One of the main problems with it is that it's dynamic and value lookup may take time. Perhaps there is a solution by which we could have member variables of a parameter set (Parameters) so that reading/writing is cheap.
A parameter system is for command line options and storing program options; it's not a replacement for member functions.
It's not a replacement for member functions in general but it should be a replacement for member functions of the kind "set_foo" that only set a parameter/option.
Being specific to "allow_extrapolation", this relates to the inner most part of Function (eval), so I don't see how the parameter system can possibly be used to handle this simply and efficiently. A user might want to change the setting at some point during execution.
Can we just make it a public bool data member of Function? That has the advantage of being named, instead of another argument. Or similarly, use a method, as @garth-wells suggests.
In the general case (e.g. for Mesh etc.), the only problem with this approach is setting 'parameters' at construction time.
Related to Function - could we possibly return a bool from Function::eval() instead of crashing, if the point lies outside the domain...
@chris_richardson I do like making simple things public member data. A downside is that it is harder to deprecate - a member function can intercept and print a deprecation message.
@garth-wells - how about a member function returning an lvalue/rvalue reference:
bool& Function::allow_extrapolation()
{ return _allow_extrapolation; }
const bool& Function::allow_extrapolation() const
{ return _allow_extrapolation; }
I think
f.allow_extrapolation(true);
is more familiar syntax than
f.allow_extrapolation() = true;
Consistency matters.
@martinal - yes, you're right.
Some thoughts:
The problem with the member function approach is that an object is not inspectable: one cannot easily print which parameters that can affect it's behavior.
If we use a member function it should be named set_allow_extrapolation
Here's another idea. The current problem is that since reading values from the parameter database is expensive (or so we fear) we want to read the value from the parameter database at some point prior to using the value but don't know when and don't know when to read the value again.
How about making the parameters member variables of a Parameters object? Using a clever macro, I think one could make it possible to combine the following:
I you believe me, I can make a sketch of a solution.
Trilinos uses a parameters system Teuchos::ParameterList to set properties on their objects. It might be worth looking at how they do it.
I don't have any problems with the simple solution:
f.allow_extrapolation(true)
Why make it more complicated? The parameter system can be used to change the default at construction.
More generally, using the parameter system is place if functions is wrong. PETSc makes extensive use of a parameter system, where at specific points parameters are checked and the appropriate function calls made. This is how we should use the parameter system.
The thing I object to is proliferation in the number of ways one can set a parameter: sometimes via a global parameter (if set prior to construction) and sometimes via a member function (if set after construction).
Agree with "set_" and "get_" names in dolfin for consistency.
The global parameters system in dolfin is not at all inspectable:
f = Function(V)
print f.parameters
<Parameter set "parameters" containing 0 parameter(s) and 0 nested parameter set(s)>
Also, per-object parameters shouldn't be global. Global parameters (even global defaults) leads to code with side effects that affects code in completely different parts of a program. This was one of the major pains when upgrading the unit tests in dolfin: tests setting parameters affected the outcome of other tests depending on their execution order.
If the reading of global (default) parameters is done as a result of an explicit call from my side, i.e.
// set to fixed defaults unaffected by parameter system
f.set_default_parameters();
// set to values read from global parameter system
f.set_parameters_from_system();
// set to values read from global parameter system with a prefix like petsc has
f.set_parameters_from_system("myprefix");
then at least I can inspect my own code and see points where it may be affected by outside influence when combined with other peoples code.
@logg I understand the wish to avoid too many get/set functions. Function doesn't have any at the moment, so I think we can live with the addition of one. They tend to pop up more in linear algebra objects, but we can handle this by grouping related parameters into single functions, e.g the relative and absolute tolerances for a solver. PETSc groups sets four solver tolerances with one set function (http://www.mcs.anl.gov/petsc/petsc-current/docs/manualpages/KSP/KSPSetTolerances.html).
Some advantages of member functions include (1) the header file documents the class; and (2) one gets tab-completion with IPython.
Back to this bug report, I'm happy with get and set functions for consistency.
@martinal I've hit bugs caused by global parameter side effects, which we need to reduce. PETSc does something along the lines of what you sketched, see http://www.mcs.anl.gov/petsc/petsc-current/docs/manualpages/Sys/PetscOptionsSetFromOptions.html.
To clarify: I'm not opposed to adding get/set functions to handle this issue, but in general I think it we should strive for an interface that does: uniform handling of parameters, inspectable objects/parameters and quick access to parameters (not needing to prefetch parameters at some unknown point in time). I believe I can suggest something not-to-complex that handles this, yet very easy to use for both defining and accessing parameters, but that can be handled as a separate issue.
Add set/get member functions to Function to allow/disallow interpolation. Fixes Issue #198.
→ <<cset 614fdaf4f683>>
Removing milestone: 1.6 (automated comment)
