Implement C++ style functors as targets for objectives

[dburov190]

This PR implements a wrapper nlopt::functor_wrapper for C++ style functors via std::function, and two new overloads of nlopt::set_min_objective, nlopt::set_max_objective.

In order to allow that, a new member field in the myfunc_data struct is added: functor_type functor;, where functor_type is defined as

typedef std::function<double(unsigned, const double*, double*)> functor_type;

This is not introduced as a pointer (like the other function-pointers are) because std::function is already a container that stores a pointer, and abstracts it away.

Important: note that the signature for the functor does not include void* data unlike all other function-pointers. That is because it is assumed that the functor already has all the data it needs.

This PR allows now to write the following:

class UserDefinedObjective {
  private:
    ImportantData data;
  public:
    UserDefinedObjective() = delete;
    UserDefinedObjective(ImportantData data) :
      data(std::move(data)) {}
    double operator()(unsigned n, const double* x, double* grad) const
    {
      // compute objective(x) and ∇objective(x) using this->data
    }
};

int main()
{
  ImportantData data;
  UserDefinedObjective objective(std::move(data));

  nlopt::opt optimizer;
  // other nlopt settings
  optimizer.set_max_objective(std::move(objective));

  optimizer.optimize(...);

  return 0;
}

Same with C++ lambdas, regular functions and even class member functions (check out std::function).

This PR also introduces a CMake macro NLOPT_add_cpp_test to quickly add cpp tests, and creates a test cpp_functor.cxx to actually test the new functionality.

Closes #219 .

[dburov190]

I'd add, I didn't implement any tests because I didn't find a consistent style, there's one file named t_tutorial.cxx -- should I add to that file? or create a new one?

Anyway, however I tested this in my own code: it compiles and runs OK.

[jschueller]

This is way better than the old way.

I'd say create another test: t_tutorial.cxx aims at reproducing the example from the documentation.
You will need to create a cmake macro to reuse these lines for another test:
https://github.com/stevengj/nlopt/blob/master/test/CMakeLists.txt#L3-L10

[dburov190]

@jschueller Done. Wrote a macro and a (somewhat) comprehensive test for the new syntax.

[dburov190]

Hm, windows build failed, says cannot find test executables... I'm sure it's my inability to write proper CMake code (: could you help me debug this one please?

[dburov190]

I looked at the failing log output again, and I can't get it why it failed. It says that t_tutorial.exe and cpp_functor.exe were not found, but testopt.exe was also not found, and I didn't change anything about that test. It also complained about private struct myfunc_data, but again, I didn't change anything except adding a new field. It would be hard to debug this for me, unfortunately, as I don't own a Windows machine (fortunately!).

[jschueller]

the compilation fails at the swig level:

nlopt\src\swig\CMakeFiles\nlopt_python.dir\nloptPYTHON_wrap.cxx(9011,38): error C2248: 'nlopt::opt::myfunc_data': cannot access private struct declared in class 'nlopt::opt' [D:\a\nlopt\nlopt\src\swig\nlopt_python.vcxproj]

[dburov190]

I suspect it was because I wrote a function alloc_myfunc_data_with_nulls() that returned a myfunc_data* pointer. I changed it to void*. Hopefully SWIG compiles now. thanks!

[dburov190]

bump
Anything else left to do?

[dburov190]

@jschueller @stevengj

Could you guys merge this in please? if everything looks OK to you

[dburov190]

bumping it again

[dburov190]

Hey, could you re-run the CI please?

[dburov190]

OK, does everything look good now?

[dburov190]

Hello?.. 🥲

[dburov190]

hi! any progress on this?

[stevengj]

My understanding is that std::move leaves the source object (objective_1) in a "valid but undefined state", which seems like not the sort of thing one would always want to do — what if the caller wants to use the objective function for something else in addition to optimization?

In general, we should be clear about the resource management here. Who is responsible for freeing the resources associated with the objective functor? Do they get freed when the functor leaves the scope where it was defined, or do they get freed when the optimizer object is destroyed?

[dburov190]

In this example, I simply used std::move just because I myself don't need the objective_1 afterwards in the main function, so it's better to move it rather than copy. But in practice, users should decide for themselves what they want: to make a copy or move the object. Since set_min_objective accepts the parameter by value, it will make a copy of the argument unless it's passed using std::move (or is an otherwise RHS expression, like nameless lambda). OK, this is a slight simplification because in reality std::function's constructor is called in between, but the copy-or-move logic is still intact -- because std::function has a move ctor.

In general, since set_min_objective, set_max_objective accept a functor by value, it is assumed that optimizer is responsible for memory management of its own copy of the functor, but the outer one should be managed by the user. I believe this is fairly common, and actually the std::move allows the user to decide whether s/he wants to keep the object or pass it altogether, instead of hardcoding this into a library. Again, in this example I chose to "move" the object because I just don't need it afterwards. But we can also write simply

optimizer.set_min_objective(objective_1);

and then the objective_1 is usable (and a copy is made).

As for the inner workings, since functor is an STL-container, functor will get destroyed when std::function's destructor is called. Which, in turn, happens when myfunc_data is freed. And that, in turn, is already well-designed within the library.

Do you agree with this approach?

[stevengj]

That seems fine to me. We shouldn't use std::move in the documentation examples, however, because people are likely to copy that blindly and will then get into trouble.

[dburov190]

oh, okay! I could change that if you want? in a new PR? and thank you for merging!

[stevengj]

A new PR to clarify this would be fine.

[heethesh]

What is unsigned n here? There are no docs for this parameter stating how to use it. Also I'm curious why const double*, double* was preferred here over std::vector<double>? The latter is better to use, I need to explicitly keep track of the array size now!

Edit: Ahhh n is the size of the data that these pointers hold!

[NogginBops]

This PR broke nlopt_algorithm_name as it relied on this ordering that was changed...

[NogginBops]

The missing , at the end of these items cause them to be merged with the item after which causes there to be one too few items in this array.