RFC: Per-variable relative tolerance criteria for x #183

aitap · 2018-05-15T16:21:58Z

Dear Steven,

Thank you for your excellent library; my specialist's thesis would be much sadder without it.

I realized that I needed different relative tolerances for the variables I was optimizing (I wanted to be sure about plasma temperature and electron density, but had much less confidence in element concentrations), so I went on and implemented that.

Why you wouldn't want to merge this right away:

I had to make interface choices just to get the code to compile. I did my best to preserve existing interface, so the new functions are inconsistently named (set_xtol_abs and set_xtol_abs1 vs set_xtol_relv and set_xtol_rel), some of them may return what can be considered lies (what should double get_xtol_rel() return when xtol_rel is a vector?), but the old behaviour is fully preserved (i.e. code using only set_xtol_rel(double) wouldn't notice any difference).
I had to change behaviour of some of the algorithms to make them compile again and to account for stop->xtol_rel being a vector. Some of the changes are nasty FIXMEs which still work when all xtol_rel elements are same but may cause suboptimal behaviour otherwise. I would need some help to decide, for example, whether it's appropriate to allocate memory for temporary copy of xtol_rel to multiply it by 10.
No attempt was made to make the new functions available from F77 or C++ before there's a decision on the interface for them.

If you are interested, can we try to work out the rest of the changes I need to implement for this to be mergeable?

aitap · 2018-07-15T08:23:28Z

Sorry to bump this, but it's been two months and I would really like to clarify a few things:

user API: should I rename the new functions for consistency with xtol_abs or should I try to preserve the old API behaviour?
internal API: what about the 5 algorithms that call nlopt_minimize* inside them? Should I update them to build an nlopt object and call nlopt_set_* on it?
Is there any interest in (eventually) merging this?

stevengj · 2018-07-27T16:09:25Z

I'm a bit skeptical about this. My inclination would be something like:

Keep the relative tolerance reltol a dimensionless scalar with the same API, but make it apply to the whole vector: the stopping criterion should be ‖Δx‖ ≤ reltol × ‖x‖, where ‖⋯‖ is some norm (probably the L1 norm for computational simplicity). This is arguably better than the current component-wise relative tolerance, which is difficult to satisfy if some components of x happen to be nearly zero.
Add a new API to specify optional weights w for the components of x. ‖x‖ will then compute the weighted L1 norm ∑|wᵢxᵢ|. (Internally, we may eventually want to just rescale the variables to yᵢ=wᵢxᵢ, since several of the algorithms implicitly assume that the components of x have similar scalings/units.)

aitap · 2018-08-04T09:17:04Z

Thank you for your reply!

I like the fact that your approach manages to preserve existing API and express what I need. I'll try to implement it without disturbing existing relstop too much, then add the weighting.

src/util/stop.c

aitap · 2018-11-09T21:17:26Z

Sorry to produce so many mistakes in the code I submit here.

I modified vector_norm and diff_norm to accept optional scaling arguments - is it still okay to handle 2*2 slightly different cases in 2 nested if clauses?

Couldn't help but notice that nlopt_stop_x and nlopt_stop_dx return 0 if a single dx[i] is > xtol_abs[i] while nlopt_stop_xs prefers to return 1 if any of the xs[i] fits the absolute stopping criterion. Is this intended?

stevengj · 2019-04-11T14:52:03Z

Couldn't help but notice that nlopt_stop_x and nlopt_stop_dx return 0 if a single dx[i] is > xtol_abs[i] while nlopt_stop_xs prefers to return 1 if any of the xs[i] fits the absolute stopping criterion. Is this intended?

That seems like a bug. Fixed in #258.

…e it const-correct

- use the weights when checking relative x stopping criterion - allow setting the weights like it's implemented for xtol_abs - weights default to 1

… functions

aitap · 2019-04-15T17:24:36Z

Thank you for clarifying the intent of nlopt_stop_xs! I have made the appropriate changes, rebased the everything on top of current master and tested resulting test/testopt for undefined behaviour (using -fsanitize=...) with relative tolerance on x (-x) enabled.

What else needs to be done here?

stevengj · 2019-04-22T15:59:07Z

src/api/options.c

@@ -157,10 +163,14 @@ nlopt_opt NLOPT_STDCALL nlopt_copy(const nlopt_opt opt)
            nopt->xtol_abs = (double *) malloc(sizeof(double) * (opt->n));
            if (!opt->xtol_abs)
                goto oom;
+            nopt->x_weights = (double *) malloc(sizeof(double) * (opt->n));


I'd rather have these initialized to NULL unless they are actually used — i.e. in the common case where the weights are equal they are not allocated. But I guess we should do the same thing for xtol_abs as well, so that could be done in a separate PR.

src/api/options.c

stevengj · 2019-04-23T17:34:38Z

It will also need an update to the documentation.

Unallocated (NULL) x_weights behave as if all weigths are set to 1.

TODO: add *_x_weights functions to C++, Fortran, Python interface, document those

stevengj · 2019-04-24T20:43:33Z

src/api/options.c

+                return NLOPT_OUT_OF_MEMORY;
+            }
+        }
+        memcpy(opt->x_weights, x_weights, opt->n * sizeof(double));


It should probably check that all of the weights are > 0 or return an INVALID_ARGS

This includes Guile and Python.

stevengj · 2019-04-25T13:26:11Z

src/api/options.c

+        unsigned i;
+        for (i = 0; i < opt->n; i++) {
+            if (x_weights[i] <= 0)
+                return NLOPT_INVALID_ARGS;


return ERR(NLOPT_INVALID_ARGS, opt, "invalid negative weight");

stevengj · 2019-04-25T13:26:59Z

src/api/options.c

@@ -654,7 +659,7 @@ nlopt_result NLOPT_STDCALL nlopt_set_x_weights(nlopt_opt opt, const double *x_we

 nlopt_result NLOPT_STDCALL nlopt_set_x_weights1(nlopt_opt opt, double x_weights)
 {
-    if (opt) {
+    if (opt && x_weights > 0) {


Better to have

if (x_weights < 0) return ERR(NLOPT_INVALID_ARGS, opt, "invalid negative weight");

so that there is an informative error message.

Should we make x_weights == 0 an error, too? I am assuming we should, since there already is xtol_rel = 0 to disable the stopping criterion.

stevengj · 2019-04-25T21:16:45Z

Looks good, I'll merge once tests are green. Thanks for sticking with this!

As discussed in stevengj#183, it is benefitical to avoid allocating potentially huge buffers of size `n` unless `x`-tolerance criteria are used.

As discussed in stevengj#183, it is beneficial to avoid allocating potentially huge buffers of size `n` unless `x`-tolerance criteria are used.

* xtol_rel: L1 norm for whole-vector stopping criterion * vector_norm: fix uninitialized ret, move branch outside for loop, make it const-correct * diff_norm: like vector_norm, but for x-oldx * xtol_abs: test directly when diff_norm tests for weighted xtol_rel * add double * x_weights to nlopt and nlopt_stopping structures - use the weights when checking relative x stopping criterion - allow setting the weights like it's implemented for xtol_abs - weights default to 1 * don't forget to free the weights; actually declare the weight-related functions * provide scale support for vector_norm and diff_norm * add argument checks to nlopt_get_x_weights * do not allocate x_weights unless requested by Unallocated (NULL) x_weights behave as if all weigths are set to 1. * start documenting the new functions TODO: add *_x_weights functions to C++, Fortran, Python interface, document those * fixes, add equations * Update NLopt_Reference.md * param names * check for w < 0 in set_x_weights * add x_weights to C++ interface and document them * add x_weights to F77 interface and document them * add & document x_weights to SWIG-based interfaces This includes Guile and Python. * add and document x_weights to Octave&Matlab * set_x_weights: ISO C90 compatibility * set_x_weights: provide informative error on w <= 0 * tweaks

* do not allocate xtol_abs unless needed As discussed in #183, it is beneficial to avoid allocating potentially huge buffers of size `n` unless `x`-tolerance criteria are used. * bobyqa: handle xtol_abs == NULL * cdirect: handle xtol_abs == NULL * cdirect/hybrid: handle xtol_abs == NULL * cobyla: handle xtol_abs == NULL * sbplx: handle xtol_abs == NULL * newuoa: handle xtol_abs == NULL * praxis: handle xtol_abs == NULL * optimize.c: handle xtol_abs == NULL

aitap force-pushed the xtol_rel_vector branch from a0b7e2d to 7828419 Compare May 15, 2018 19:45

aitap changed the title ~~Per-variable relative tolerance criteria for x~~ RFC: Per-variable relative tolerance criteria for x May 30, 2018

aitap force-pushed the xtol_rel_vector branch from 7e26441 to baa482a Compare July 26, 2018 19:26

aitap force-pushed the xtol_rel_vector branch from baa482a to b138344 Compare August 4, 2018 09:11

stevengj reviewed Aug 6, 2018

View reviewed changes