nlopt
nlopt copied to clipboard
`nlopt::get_initial_step` overloads always throw `std::invalid_argument`
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.