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:

nlopt/src/api/nlopt-in.hpp

Lines 41 to 43 in b2caea2

	inline nlopt_result nlopt_get_initial_step(const nlopt_opt opt, double *dx) {
	return nlopt_get_initial_step(opt, (const double *) NULL, dx);
	}

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:

nlopt/src/api/options.c

Line 892 in b2caea2

nlopt_result NLOPT_STDCALL nlopt_get_initial_step(const nlopt_opt opt, const double *x, double *dx)

within the same function, a few lines down,

nlopt/src/api/options.c

Line 901 in b2caea2

nlopt_result ret = nlopt_set_default_initial_step(o, x);

And within nlopt_set_default_initial_step, there is this check:

nlopt/src/api/options.c

Lines 918 to 919 in b2caea2

	if (!opt || !x)
	return NLOPT_INVALID_ARGS;

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,

nlopt/src/api/nlopt-in.hpp

Line 580 in b2caea2

NLOPT_GETSET_VEC(initial_step)

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 596799b to only document the functional method for now.