search

alashworth / test-issue-import

0 stars 0 forks source link

Optimization return code is misleading #124

Open alashworth opened 5 years ago

alashworth commented 5 years ago

Issue by sakrejda Friday Dec 30, 2016 at 15:58 GMT Originally opened as https://github.com/stan-dev/stan/issues/2197

Summary:

In do_bfgs_optimize, the return code is still positive even if the algorithm stops because it reaches max iterations. That leads stan to return stan::servicse::error_codes::OK even though the algorithm did not meet stopping criteria. I don't think this is necessarily wrong but it is misleading.

Description:

The return code (ret below) will be positive even if the algorithm terminates because it reaches max iterations. I don't think Stan should treat that the same as meeting convergence diagnostics. Relevant snippet from do_bfgs.hpp: 

        if (ret >= 0) {
          info("Optimization terminated normally: ");
          return_code = stan::services::error_codes::OK;
        } else {
          info("Optimization terminated with error: ");
          return_code = stan::services::error_codes::SOFTWARE;
        }
info("  " + bfgs.get_code_string(ret));

Reproducible Steps:

It's pretty clear from the code. Did not try to make an example. Probably a model like the following would do it.

parameters {
  real x;
}
model {
  target += log(x);
}

Current Output:

The message that the algorithm terminated because it reached max iterations is only written on a side-channel ("info"), the Stan return value is still stan::services::error_codes::OK

Expected Output:

Terminating because the algorithm runs out of iterations should give some return value other than "OK"

Current Version:

v2.14.0

alashworth commented 5 years ago

Comment by sakrejda Friday Dec 30, 2016 at 16:03 GMT

I guess the return for running out of iterations should either be DATAERR (misspecified model) or CONFIG (not enough iterations).

alashworth commented 5 years ago

Comment by bob-carpenter Friday Dec 30, 2016 at 17:06 GMT

Why not add a specific return code for running out of iterations?

Where are DATAERR and CONFIG defined?

alashworth commented 5 years ago

Comment by sakrejda Friday Dec 30, 2016 at 17:32 GMT

I do think it would make sense to have our own error codes and be more specific for each algorithm but currently it sounds as though we're limiting ourselves: https://github.com/stan-dev/stan/blob/develop/src/stan/services/error_codes.hpp, not sure who made the decision.

alashworth commented 5 years ago

Comment by syclik Friday Dec 30, 2016 at 17:57 GMT

I believe that was me at some point after seeing some of the code returning 0, 100, -1, -2, -1000, and some other funky numbers.

There are two things here: 1) return codes and 2) information about errors. They can be, but in no way must be the same.

Return codes from applications just need to follow a simple rule: 0 for success, not zero for failure. What constitutes a failure is up to us to decide. The error codes were put in when these return codes was what was returned from int main() from CmdStan.

Information from errors can be handled in lots of ways including exceptions, error codes, messages, etc. I personally haven't found error codes to be that useful and would rather throw a named exception with a usable error message.

I guess what I'd like to see is what error conditions you expect to see out of the optimization functions, what information we need to propagate regarding the error, and what you expect the application to return when those errors occur.

On Dec 30, 2016, at 12:32 PM, Krzysztof Sakrejda notifications@github.com wrote:

I do think it would make sense to have our own error codes and be more specific for each algorithm but currently it sounds as though we're limiting ourselves: https://github.com/stan-dev/stan/blob/develop/src/stan/services/error_codes.hpp, not sure who made the decision.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

alashworth commented 5 years ago

Comment by sakrejda Friday Dec 30, 2016 at 19:32 GMT

@syclik I'd rather not make this small issue about re-designing how Stan does return codes, just fixing this one problem. I made a separate issue for a re-design:

alashworth commented 5 years ago

Comment by syclik Friday Dec 30, 2016 at 19:39 GMT

Ok. In that case, just submit a pull request for the else branch in the original post. Remember, the way this is currently working is that the return code is what's actually returned from int main(), so it is literally the return code from the program in CmdStan.

Honestly, I'm looking at the sysexits page that's linked there and software is the closest things that matches. It seems like the correct behavior: 0 for success, non 0 for failure.

What are you suggesting happens for this issue?

alashworth commented 5 years ago

Comment by syclik Friday Dec 30, 2016 at 19:43 GMT

I'm looking at the original post again. I think you're suggesting that the algorithm failed if it completed all its iterations. I'm not sure that constitutes a failure. The algorithm actually did what it was supposed to do.

alashworth commented 5 years ago

Comment by sakrejda Friday Dec 30, 2016 at 19:46 GMT

I am suggesting that "OK" as a return code for completing all the iterations is misleading but I guess you're right that what exactly it should return is debatable. It should return something other than "OK", so by necessity a failing (non-zero) return code. If you think it's better to just leave this as-is that's fine, you can close the issue.

alashworth commented 5 years ago

Comment by syclik Friday Dec 30, 2016 at 20:10 GMT

I'd like to know how you're proposing to change this. I don't actually understand what you're suggesting with this issue. What constitutes a success and a failure is debatable, so if you have an idea of where that line should be, please be clear about it and we can adjust accordingly.

alashworth commented 5 years ago

Comment by sakrejda Friday Dec 30, 2016 at 20:33 GMT

Trying to be clearer here, I'm suggesting changing from:

        if (ret >= 0) {
          info("Optimization terminated normally: ");
          return_code = stan::services::error_codes::OK;
        } else {
          info("Optimization terminated with error: ");
          return_code = stan::services::error_codes::SOFTWARE;
        }

to 

        if (ret < 0 || ret == 40) {    // LSFAIL or MAXIT returns
          info("Optimization not completed: ");
          return_code = stan::services::error_codes::SOFTWARE; 
        } else {
          info("Optimization terminated normally: ");
          return_code = stan::services::error_codes::OK;
        }

The argument for this change is that this gets used as a return code in CmdStan and somebody who runs a CmdStan program probably expects "OK"/0 to mean that the local optimum was achieved.

When using CmdStan optimization as an optimization sub-step in some bigger algorithm it might make sense to treat MAXIT as a success but that's a corner case and should be treated as such.

alashworth commented 5 years ago

Comment by bgoodri Friday Dec 30, 2016 at 20:54 GMT

Stan should return the same error codes as in the original LBFGS fortran code.

On Fri, Dec 30, 2016 at 3:33 PM, Krzysztof Sakrejda < notifications@github.com> wrote:

Trying to be clearer here, I'm suggesting changing from:

    if (ret >= 0) {
      info("Optimization terminated normally: ");
      return_code = stan::services::error_codes::OK;
    } else {
      info("Optimization terminated with error: ");
      return_code = stan::services::error_codes::SOFTWARE;
    }

to

    if (ret < 0 || ret == 40) {    // LSFAIL or MAXIT returns
      info("Optimization not completed: ");
      return_code = stan::services::error_codes::SOFTWARE;
    } else {
      info("Optimization terminated normally: ");
      return_code = stan::services::error_codes::OK;
    }

The argument for this change is that this gets used as a return code in CmdStan and somebody who runs a CmdStan program probably expects "OK"/0 to mean that the local optimum was achieved.

When using CmdStan optimization as an optimization sub-step in some bigger algorithm it might make sense to treat MAXIT as a success but that's a corner case and should be treated as such.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2197#issuecomment-269818835, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqstEwhZpfy9HfOozVkJny2-bc2veks5rNWq1gaJpZM4LYQVz .

alashworth commented 5 years ago

Comment by sakrejda Friday Dec 30, 2016 at 21:11 GMT

Stan should return the same error codes as in the original LBFGS fortran code.

That's out of left field. Why should CmdStan be stuck with return codes from some other fortran code? Could you explain what you mean?

alashworth commented 5 years ago

Comment by bgoodri Friday Dec 30, 2016 at 21:25 GMT

I mean if someone is calling LBFGS, their default expectation is going to be that it behaves the same way as LBFGS everywhere else.

alashworth commented 5 years ago

Comment by sakrejda Friday Dec 30, 2016 at 21:37 GMT

My suggested change does not affect that. Current behavior does not follow your suggestion either. I agree with the broader sentiment but I think it's irrelevant here as current behavior does not follow your suggestion.

On Fri, Dec 30, 2016, 4:25 PM bgoodri notifications@github.com wrote:

I mean if someone is calling LBFGS, their default expectation is going to be that it behaves the same way as LBFGS everywhere else.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2197#issuecomment-269823869, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfA6RpUwOj64sVFyZydVquEByFNFEVHks5rNXbSgaJpZM4LYQVz .

alashworth commented 5 years ago

Comment by syclik Monday Jan 02, 2017 at 01:47 GMT

@bgoodri, what do you mean? What particular implementation are you talking about? Can you point me to the doc?

alashworth commented 5 years ago

Comment by bgoodri Monday Jan 02, 2017 at 04:16 GMT

Nocedal's: http://users.iems.northwestern.edu/~nocedal/lbfgs.html

C IFLAG is an INTEGER variable that must be set to 0 on initial entry

C to the subroutine. A return with IFLAG<0 indicates an error, C and IFLAG=0 indicates that the routine has terminated without C detecting errors. On a return with IFLAG=1, the user must C evaluate the function F and gradient G. On a return with C IFLAG=2, the user must provide the diagonal matrix Hk0. C C The following negative values of IFLAG, detecting an error, C are possible: C C IFLAG=-1 The line search routine MCSRCH failed. The C parameter INFO provides more detailed information C (see also the documentation of MCSRCH): C C INFO = 0 IMPROPER INPUT PARAMETERS. C C INFO = 2 RELATIVE WIDTH OF THE INTERVAL OF C UNCERTAINTY IS AT MOST XTOL. C C INFO = 3 MORE THAN 20 FUNCTION EVALUATIONS WERE C REQUIRED AT THE PRESENT ITERATION. C C INFO = 4 THE STEP IS TOO SMALL. C C INFO = 5 THE STEP IS TOO LARGE. C C INFO = 6 ROUNDING ERRORS PREVENT FURTHER PROGRESS. C THERE MAY NOT BE A STEP WHICH SATISFIES C THE SUFFICIENT DECREASE AND CURVATURE C CONDITIONS. TOLERANCES MAY BE TOO SMALL. C C C IFLAG=-2 The i-th diagonal element of the diagonal inverse C Hessian approximation, given in DIAG, is not C positive. C C IFLAG=-3 Improper input parameters for LBFGS (N or M are C not positive).

On Sun, Jan 1, 2017 at 8:47 PM, Daniel Lee notifications@github.com wrote:

@bgoodri https://github.com/bgoodri, what do you mean? What particular implementation are you talking about? Can you point me to the doc?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2197#issuecomment-269929020, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqonZb1Tye6aBhetniOFsDULpDEMfks5rOFcqgaJpZM4LYQVz .