Stan's machine-readable return types (as opposed to its free-form messages) conflict.

alashworth commented 5 years ago

Issue by sakrejda Friday Dec 30, 2016 at 19:31 GMT Originally opened as https://github.com/stan-dev/stan/issues/2198

Summary:

Stan defines machine-readable return types (error codes, exceptions, ....??) in a variety of places and they don't necessarily agree.

Description:

1) [stan::services::error_codes](

namespace stan {
  namespace services {

    struct error_codes {
      // defining error codes to follow FreeBSD sysexits conventions
      // http://www.gsp.com/cgi-bin/man.cgi?section=3&topic=sysexits
      enum {
        OK = 0,
        USAGE = 64,
        DATAERR = 65,
        NOINPUT = 66,
        SOFTWARE = 70,
        CONFIG = 78
      };
    };
  }

2) (stan::optimization::TerminationCondition)[https://github.com/stan-dev/stan/blob/develop/src/stan/optimization/bfgs.hpp]


namespace stan {
  namespace optimization {
    typedef enum {
      TERM_SUCCESS = 0,
      TERM_ABSX = 10,
      TERM_ABSF = 20,
      TERM_RELF = 21,
      TERM_ABSGRAD = 30,
      TERM_RELGRAD = 31,
      TERM_MAXIT = 40,
      TERM_LSFAIL = -1
    } TerminationCondition;

3) The stan::mcmc code seems to use exceptions not error codes. 4) stan::variational seems to use free-form messages (?) and some stan::services::errror_codes.

Expected Output:

Writing a suggestion below but I'd be fine with a variety of solutions. We could just always return 0 and throw on all clear errors. Figuring out if the output is good enough to use is a little more advanced than typical error codes should be used to classify.

Return codes are fine for typical operation (the output may be useful) and exceptions for atypical conditions (output probably not what you were looking for, no clear way for the algorithm to continue).
We should define return codes in one place stan::services::return_codes or similar. We should not standardize on FreeBSD sysexits conventions (as stan::services::error_codes currently does) since those conventions are for command-line programs and they understandably do not cover the variety of possible return conditions Stan produces.
Return codes should be kept in a single file (so they don't conflict) but they should be algorithm-specific. I'd be fine with something simple like saving 0XX for later use, 1XX for optimization, 2XX for NUTS, 3XX for NUTS with adaptation, 4XX for HMC, 5XX for variational inference, something like that. If you plan on having more than 99 error codes for one algorithm you're doing it wrong.
There should only be a few return codes per algorithm, stan::optimization seems to get this right (?)
It would be fine to continue to provide stan::services::error_codes following FreeBSD sysexit conventions for command-line programs written with stan but that's a separate issue.
optimization should throw for the case currently handled by TERM_LSFAIL (line search failed) and generally be changed to use exceptions for error.

Current Version:

v2.14.0

alashworth commented 5 years ago

Comment by ariddell Saturday Dec 31, 2016 at 23:04 GMT

Good catch. My sense was that errors are typically communicated with exceptions. Having to think about exceptions and error-indicating return codes (rather than just one of the two) seems like a unnecessary burden on the developer.

alashworth commented 5 years ago

Comment by ariddell Monday Jan 02, 2017 at 13:40 GMT

We probably all agree that they (error messages/codes) should at least not conflict/overlap without ample documentation. Is unconflicting them a discrete task? If we've got most error information being communicated with exceptions, it seems reasonable to default to communicating error information that way.

alashworth commented 5 years ago

Comment by sakrejda Monday Jan 02, 2017 at 13:48 GMT

@bgoodri @syclik I think Ben's comment is more relevant for this issue than the dther one so I'm adding it here. It looks like we only use one of the error codes and (maybe?) throw for some of the other conditions.

The implementation of L-BFGS by Jorge Nocedal has documented codes that it would be useful to stick to. They are documented in the package distributed at: http://users.iems.northwestern.edu/~nocedal/lbfgs.html

From code at that link:

IFLAG is an INTEGER variable that must be set to 0 on initial entry to the subroutine. A return with IFLAG<0 indicates an error, and IFLAG=0 indicates that the routine has terminated without detecting errors. On a return with IFLAG=1, the user must evaluate the function F and gradient G. On a return with IFLAG=2, the user must provide the diagonal matrix Hk0.

The following negative values of IFLAG, detecting an error, are possible:

IFLAG=-1 The line search routine MCSRCH failed. The parameter INFO provides more detailed information (see also the documentation of MCSRCH):

INFO = 0 IMPROPER INPUT PARAMETERS.

INFO = 2 RELATIVE WIDTH OF THE INTERVAL OF UNCERTAINTY IS AT MOST XTOL.

INFO = 3 MORE THAN 20 FUNCTION EVALUATIONS WERE REQUIRED AT THE PRESENT ITERATION.

INFO = 4 THE STEP IS TOO SMALL.

INFO = 5 THE STEP IS TOO LARGE.

INFO = 6 ROUNDING ERRORS PREVENT FURTHER PROGRESS. THERE MAY NOT BE A STEP WHICH SATISFIES THE SUFFICIENT DECREASE AND CURVATURE CONDITIONS. TOLERANCES MAY BE TOO SMALL.

IFLAG=-2 The i-th diagonal element of the diagonal inverse Hessian approximation, given in DIAG, is not positive.

IFLAG=-3 Improper input parameters for LBFGS (N or M are not positive).

alashworth commented 5 years ago

Comment by sakrejda Monday Jan 02, 2017 at 14:02 GMT

@ariddell The coding would be straightforward so I made it an issue rather than trying to discuss on the board.

My suggestion is:

throw on failures (broadly defined) and pass the various IFLAG/INFO-type (from L-BFGS) information through those exceptions. People using something like L-BFGS would benefit from having the typical return flags but they don't need them specifically as return codes. It would be trivial to write a wrapper that catches the exceptions and returns the standard flags if somebody wants a plug-in replacement for the standard L-BFGS.
on success (i.e.-not failing any internal diagnostics) provide one way (shared among algorithms) to query the algorithm object for what kind of success it is (L-BFGS has some flags for this too so it's a good example to look at).
on success return a bool true (since that's pretty idiomatic for c++) and on undiagnosed failure (i.e.-a developer forgot to add an internal throw for a failure condition) return false.

alashworth commented 5 years ago

Comment by ariddell Monday Jan 02, 2017 at 15:25 GMT

Sounds good to me. I like the idea of being consistent: if we are communicating information via exceptions when NUTS or VB errors occur, we should do the same thing with optimizing.

alashworth commented 5 years ago

Comment by bob-carpenter Tuesday Jan 03, 2017 at 20:07 GMT

The utility of any warning message or return code is what the client receiving it (command line user, other process that execs) can do with it. If the client can't correct the problem and keep going, it just needs to rethrow or return its own error code.

From a C++ perspective, all those functions with return codes in stan-dev/stan should be declared with

void return type
exceptions for error conditions

But, if we do that, then clients like CmdStan would have to catch and interpret those exceptions in order to implement the same return codes as commands as L-BFGS. But I don't think we'll get interoperability at that level anyway, so I don't think we should worry about it.

We can use it to figure out what kinds of warning conditions and error conditions to flag.

Bob

On Jan 2, 2017, at 10:25 AM, Allen Riddell notifications@github.com wrote:

Sounds good to me. I like the idea of being consistent: if we are communicating information via exceptions when NUTS or VB errors occur, we should do the same thing with optimizing.

— 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 / test-issue-import