`nlopt::get_initial_step` overloads always throw `std::invalid_argument`

[alexriegler]

I've noticed that void nlopt::get_initial_step(std::vector<double> &v) and std::vector<double> nlopt::get_initial_step() are unusable in their current state. Right now, they always throw std::invalid_argument.

Description

I dug into the issue a little bit and it seems the issue is related to the overloads of get_initial_step that do not take an initial guess (these are also the overloads defined by the NLOPT_GETSET_VEC macro). These functions call this overload:

https://github.com/stevengj/nlopt/blob/b2caea26d9fb88884a3683bd5a0a4e09f2e59199/src/api/nlopt-in.hpp#L41-L43

The issue is that the overload above provides an invalid argument to nlopt_get_initial_step, that argument being NULL. This argument is considered invalid because nlopt_get_initial_step frowards the NULL value to nlopt_set_default_initial_step:

https://github.com/stevengj/nlopt/blob/b2caea26d9fb88884a3683bd5a0a4e09f2e59199/src/api/options.c#L892

within the same function, a few lines down,

https://github.com/stevengj/nlopt/blob/b2caea26d9fb88884a3683bd5a0a4e09f2e59199/src/api/options.c#L901

And within nlopt_set_default_initial_step, there is this check:

https://github.com/stevengj/nlopt/blob/b2caea26d9fb88884a3683bd5a0a4e09f2e59199/src/api/options.c#L918-L919

Which will evaluate to true and propagate the error back up to the caller as a std::invalid_argument exception.

Proposed fix

My suggestion would be to remove the two problematic functions. Given that they do not work as is, I can't imagine removing the functions would negatively affect any users.

To remove the functions, I suggest removing the usage of the NLOPT_GETSET_VEC macro for initial_step and then manually defining the setters or, to be consistent with prior code, create a NLOPT_SET_VEC macro to define them for you.

In other words, replace,

https://github.com/stevengj/nlopt/blob/b2caea26d9fb88884a3683bd5a0a4e09f2e59199/src/api/nlopt-in.hpp#L580

with,

Option 1 (manual definition):

void set_initial_step(double val)
{
  mythrow(nlopt_set_initial_step1(o, val));
}
void set_initial_step(const std::vector<double> &v)
{
  if (o && nlopt_get_dimension(o) != v.size())
    throw std::invalid_argument("dimension mismatch");
  mythrow(nlopt_set_initial_step(o, v.empty() ? NULL : &v[0]));
}

or,

Option 2 (macro):

#define NLOPT_SET_VEC(name)                                 \
  void set_##name(double val)                               \
  {                                                         \
    mythrow(nlopt_set_##name##1(o, val));                   \
  }                                                         \
  void set_##name(const std::vector<double> &v)             \
  {                                                         \
    if (o && nlopt_get_dimension(o) != v.size())            \
      throw std::invalid_argument("dimension mismatch");    \
    mythrow(nlopt_set_##name(o, v.empty() ? NULL : &v[0])); \
  }

// ...

NLOPT_SET_VEC(initial_step)

If you go with Option 2, you might as well update the NLOPT_GETSET_VEC macro to the following,

#define NLOPT_GETSET_VEC(name)                              \
  NLOPT_SET_VEC(name)                                       \
  void get_##name(std::vector<double> &v) const             \
  {                                                         \
    if (o && nlopt_get_dimension(o) != v.size())            \
      throw std::invalid_argument("dimension mismatch");    \
    mythrow(nlopt_get_##name(o, v.empty() ? NULL : &v[0])); \
  }                                                         \
  std::vector<double> get_##name() const                    \
  {                                                         \
    if (!o)                                                 \
      throw std::runtime_error("uninitialized nlopt::opt"); \
    std::vector<double> v(nlopt_get_dimension(o));          \
    get_##name(v);                                          \
    return v;                                               \
  }

Additionally, relevant documentation should be updated.

[stevengj]

Right, we should probably get rid of that get function, and replace it with a call to get_initial_step_ which allocates the result.

Note, however, that there is a usable getter function get_initial_step(const std::vector<double> &x, std::vector<double> &dx) where you pass an array dx to hold the result. This one is documented in the manual.

I fixed the documentation in 596799bfb88088612de110ba1aa8cf5fcd89ae82 to only document the functional method for now.