-
-
Notifications
You must be signed in to change notification settings - Fork 368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimization return code is misleading #2197
Comments
I guess the return for running out of iterations should either be DATAERR (misspecified model) or CONFIG (not enough iterations). |
Why not add a specific return code for running out of iterations?
Where are `DATAERR` and `CONFIG` defined?
|
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. |
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 ***@***.***> 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.
|
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 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? |
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. |
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. |
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. |
Trying to be clearer here, I'm suggesting changing from:
to
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. |
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 < ***@***.***> 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
<#2197 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqstEwhZpfy9HfOozVkJny2-bc2veks5rNWq1gaJpZM4LYQVz>
.
|
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? |
I mean if someone is calling LBFGS, their default expectation is going to
be that it behaves the same way as LBFGS everywhere else.
|
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 ***@***.***> 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
<#2197 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAfA6RpUwOj64sVFyZydVquEByFNFEVHks5rNXbSgaJpZM4LYQVz>
.
|
@bgoodri, what do you mean? What particular implementation are you talking about? Can you point me to the doc? |
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 ***@***.***> 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
<#2197 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqonZb1Tye6aBhetniOFsDULpDEMfks5rOFcqgaJpZM4LYQVz>
.
|
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: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.
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
The text was updated successfully, but these errors were encountered: