siconos / siconos

Simulation framework for nonsmooth dynamical systems
https://nonsmooth.gricad-pages.univ-grenoble-alpes.fr/siconos/
Apache License 2.0
162 stars 30 forks source link

string based parameters in SolverOptions #91

Closed xhub closed 6 years ago

xhub commented 7 years ago

Right now we have the following mechanism to specify option for the solver: dparam and iparam in SolverOptions. It works fine for some basic option handling, but has some shortcomings: it is inconsistent across solvers (like the number of iteration done is sometimes in iparam[1] or iparam[7]). Also there is an overlap between solvers, which makes it difficult to combine them, like in hybrid solvers.

I propose that we add 4 new fields to SolverOptions, for passing double, integer, booleans ans string options. Each option would then be identify by a string, which is easier to make unique rather than a number.

I already did something similar in tools/GAMSlink.h, with the GAMSopt* options. It is based on a linked list approach. Each option has a string identifier, a value, and a type.

We could keep dparam and iparam for the basic options like number of iteration, tolerance, ... But for the more solver-specific ones, we could migrate to the new system. This approach would also enable us to easily set the linear algebra routines in an solver-agnostic way.

I believe this is how other optimization solver handle options.

vacary commented 7 years ago

Hi,

I am not in favor of inserting string options in SolverOptions. It was like that 5/6 years ago and we remove it to strenghen his interface.

I am more in favor of using ENUM as we have started to to in fc3d_nsgs. This solution enables uniformization of parameters and typos can be checked easily in a static way at the compilation.

Not sure than the low-level optimization codes handles the option in this way.

radarsat1 commented 7 years ago

The other possibility that is also statically checkable by the compiler is to use union. You identify which union sub-structure to use based on the solverId. That way instead of having,

solver_options.iparam[SOME_NSGS_IPARAM_ENUM] = 0;

you would have,

if (solver_options.solverId == NSGS) {
  solver_options.nsgs.some_option = 0;
}

which is quite clean and readable imho.

radarsat1 commented 7 years ago

Forgot to note, the other major advantage with using sub-structures is that you have the full power of a struct for describing the options, so the compiler can verify int/double mismatches etc.

xhub commented 7 years ago

I think that the idea of a having a struct for the option has grown on me. I still think that having the option to have string options would be useful. Checking that there is no typo and that the data type matches should be possible with a mechanism to register the option and do some checking (via X-macro even via static_assert for the code inside numerics). But if we reach a consensus on a struct, that's already a big step forward from the current state.

What I want to be able to do is to specify:

I feel that the dparam and iparam does not allow me to do that.

radarsat1 commented 7 years ago

Yeah, I think having named structs and /or strings would go a long way towards having a specification too. I'm going to need eventually some structured way of specifying Siconos-related options in the physics parameters for Gazebo XML files (SDF).

xhub commented 7 years ago

Yeah, I think having named structs and /or strings would go a long way towards having a specification too. I'm going to need eventually some structured way of specifying Siconos-related options in the physics parameters for Gazebo XML files (SDF).

Could you try to draft a spec for that? Having a concrete example could help us design something good.

radarsat1 commented 7 years ago

Eventually, yes. It'll just be an encoding of what we are already doing with respect to the simulation and solver parameters. It's on the roadmap. I was thinking the other way around, that defining some data structures might help towards drafting a spec ;)

In any case I'll try to find some time to try the "struct" idea on the NSGS parameters for example, unfortunately it is not be too obvious to me how to integrate such a change a piece at a time, it seems like we should convert all solver parameters to structs or strings all at once. Maybe we'll have to experiment on a branch?

The main difficulty I think is determining what options are in common among solvers or classes of solvers and which are specific to particular solvers, and making a "common options" struct. I would suggest that "common options" struct is contained at the beginning of each options struct so that a union like this is valid:

struct nsgs_options {
    struct solver_options;
    float an_extra_option;
    int another_option;
};

union {
    struct solver_options;
    struct nsgs_options;
};

That is to say that accessing the union.common_options is always valid, but accessing nsgs_options is only valid if it's the correct type.

It might be wise also to store the size and/or type of the struct somewhere for assertions and debugging. I've seen plenty of instances in C APIs where the user must do:

my_nsgs_options->typetag = NSGS_OPTIONS;
my_nsgs_options->size = sizeof(nsgs_options);

before passing the pointer somewhere, and later this can be used to check that we aren't accessing past the end of the struct when copying it for example.

Anyway, it would be best to try it out and see what seems to work for numerics.

vacary commented 7 years ago

I am still not convinced, but perhaps, we can find a good example of this way of doing in other software. sundials ? matlab ?

I find the enum strategy quite good and I want to generalize it in Siconos. For the other options and info, we have to think how to send user_data with a agnostic structure.

options for the linear solver (solver to use, number of iterative refinements + criterion, number of CPU to use, type of ordering + permutation, ...)

I am doing that with iparam and param and a naming convention given by enum for instance:

`enum SICONOS_FRICTION_3D_NSN_DPARAM
{
  /** index in dparam to store the rho value for porjection formulation */
  SICONOS_FRICTION_3D_NSN_RHO = 3,

};
enum SICONOS_FRICTION_3D_NSN_FORMULATION_ENUM
{
  SICONOS_FRICTION_3D_NSN_FORMULATION_ALARTCURNIER_STD =0,
  SICONOS_FRICTION_3D_NSN_FORMULATION_JEANMOREAU_STD = 1,
  SICONOS_FRICTION_3D_NSN_FORMULATION_ALARTCURNIER_GENERATED =2,
  SICONOS_FRICTION_3D_NSN_FORMULATION_JEANMOREAU_GENERATED =3,
  SICONOS_FRICTION_3D_NSN_FORMULATION_NULL = 4 ,
};

enum SICONOS_FRICTION_3D_NSN_LINESEARCH_ENUM
{
  SICONOS_FRICTION_3D_NSN_LINESEARCH_GOLDSTEINPRICE = 0 ,
  SICONOS_FRICTION_3D_NSN_LINESEARCH_ARMIJO = 1,
  SICONOS_FRICTION_3D_NSN_LINESEARCH_NO=-1,
};

enum SICONOS_FRICTION_3D_NSN_HYBRID_ENUM
{
  /** Loop NSN-PLI strategy for the hybrid solver */
  SICONOS_FRICTION_3D_NSN_HYBRID_STRATEGY_NO = 0,
  /** Loop NSN-PLI strategy for the hybrid solver */
  SICONOS_FRICTION_3D_NSN_HYBRID_STRATEGY_PLI_NSN_LOOP = 1,
  /** Loop PLI-NSN-PLI strategy for the hybrid solver */
  SICONOS_FRICTION_3D_NSN_HYBRID_STRATEGY_NSN_PLI_LOOP = 2,
  /** NSN and after Loop NSN-PLI strategy for the hybrid solver */
  SICONOS_FRICTION_3D_NSN_HYBRID_STRATEGY_NSN_AND_NSN_PLI_LOOP = 3,
  SICONOS_FRICTION_3D_NSN_HYBRID_STRATEGY_VI_EG_NSN =4,
};`

options for the logger: I log some data using hdf5 (because it is so much better than looking at stdout). I need to specify a filename and log level.

Should we put that in the options ?

solver-specific option

Why the actual system is not sufficient ?

I am afraid about the fact that if we change the type of structure for each solver/config, we will need to manage static cast in the driver or to define fat structures with a lot of useless entries.

I think that a draft of the structure may help to convince me. In particular on the fact that the driver will keep its API.

xhub commented 7 years ago

Postpone this idea.