Eliminate error message when sampling

[syclik]

Users have difficulty understanding the rejection messages while sampling.

Perhaps some of the issue is due to the "error" within the message itself. Perhaps we should reword that bit.

We should also eliminate the error message.

[syclik]

I found out where the "Error in function:" comes from. It's in boost/math/policies/error_handling.hpp, inside the raise_error function.

We already reimplemented most of the exception handling, so perhaps we should bypass Boost's error handling altogether.

[betanalpha]

Yeah, I think Bob summarized it best: what happens is that the log_prob function throws an error,
the sampler catches it and downgrades it to a warning but displays the original error message
including the apparently indecipherable “Error in function” bit.

On May 7, 2014, at 10:54 PM, Daniel Lee notifications@github.com wrote:

I found out where the "Error in function:" comes from. It's in boost/math/policies/error_handling.hpp, inside the raise_error function.

We already reimplemented most of the exception handling, so perhaps we should bypass Boost's error handling altogether.

—
Reply to this email directly or view it on GitHub.

[syclik]

If that's what's causing the confusion, we could reimplement that portion
of it, have the same behavior, but different message.

I'm not a fan of a warning message saying that there's an error. No wonder
it's causing confusion.

On Wed, May 7, 2014 at 6:07 PM, Michael Betancourt <notifications@github.com

wrote:

Yeah, I think Bob summarized it best: what happens is that the log_prob
function throws an error,
the sampler catches it and downgrades it to a warning but displays the
original error message
including the apparently indecipherable “Error in function” bit.

On May 7, 2014, at 10:54 PM, Daniel Lee notifications@github.com wrote:

I found out where the "Error in function:" comes from. It's in
boost/math/policies/error_handling.hpp, inside the raise_error function.

We already reimplemented most of the exception handling, so perhaps we
should bypass Boost's error handling altogether.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/640#issuecomment-42489702
.

[bob-carpenter]

If we catch all the errors, then it should be OK to turn
off the Boost error handling. But apparently we're not
catching this one ourselves if the error's coming from Boost.

A very crude fix would be to have the informational message
just remove the embedded "Error" and replace it with "Difficulty"
or something like that --- it's easy enough with string
manipulation.

• Bob

On May 8, 2014, at 12:15 AM, Daniel Lee notifications@github.com wrote:

If that's what's causing the confusion, we could reimplement that portion
of it, have the same behavior, but different message.

I'm not a fan of a warning message saying that there's an error. No wonder
it's causing confusion.

On Wed, May 7, 2014 at 6:07 PM, Michael Betancourt <notifications@github.com

wrote:

Yeah, I think Bob summarized it best: what happens is that the log_prob
function throws an error,
the sampler catches it and downgrades it to a warning but displays the
original error message
including the apparently indecipherable “Error in function” bit.

On May 7, 2014, at 10:54 PM, Daniel Lee notifications@github.com wrote:

I found out where the "Error in function:" comes from. It's in
boost/math/policies/error_handling.hpp, inside the raise_error function.

We already reimplemented most of the exception handling, so perhaps we
should bypass Boost's error handling altogether.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/640#issuecomment-42489702
.

—
Reply to this email directly or view it on GitHub.

[betanalpha]

We’re catching it but repeating the original error verbatim.
I think what we really want to do is separate the level of the
issue (warning/error) from the description (NaN, 0, etc).

On May 7, 2014, at 11:28 PM, Bob Carpenter notifications@github.com wrote:

If we catch all the errors, then it should be OK to turn
off the Boost error handling. But apparently we're not
catching this one ourselves if the error's coming from Boost.

A very crude fix would be to have the informational message
just remove the embedded "Error" and replace it with "Difficulty"
or something like that --- it's easy enough with string
manipulation.

• Bob

On May 8, 2014, at 12:15 AM, Daniel Lee notifications@github.com wrote:

If that's what's causing the confusion, we could reimplement that portion
of it, have the same behavior, but different message.

I'm not a fan of a warning message saying that there's an error. No wonder
it's causing confusion.

On Wed, May 7, 2014 at 6:07 PM, Michael Betancourt <notifications@github.com

wrote:

Yeah, I think Bob summarized it best: what happens is that the log_prob
function throws an error,
the sampler catches it and downgrades it to a warning but displays the
original error message
including the apparently indecipherable “Error in function” bit.

On May 7, 2014, at 10:54 PM, Daniel Lee notifications@github.com wrote:

I found out where the "Error in function:" comes from. It's in
boost/math/policies/error_handling.hpp, inside the raise_error function.

We already reimplemented most of the exception handling, so perhaps we
should bypass Boost's error handling altogether.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/640#issuecomment-42489702
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[bob-carpenter]

How can we do that? The error gets thrown in the context
of the function evaluation, which doesn't know its context.
Catch and rethrow with a different message?

• Bob

On May 8, 2014, at 12:33 AM, Michael Betancourt notifications@github.com wrote:

We’re catching it but repeating the original error verbatim.
I think what we really want to do is separate the level of the
issue (warning/error) from the description (NaN, 0, etc).

On May 7, 2014, at 11:28 PM, Bob Carpenter notifications@github.com wrote:

If we catch all the errors, then it should be OK to turn
off the Boost error handling. But apparently we're not
catching this one ourselves if the error's coming from Boost.

A very crude fix would be to have the informational message
just remove the embedded "Error" and replace it with "Difficulty"
or something like that --- it's easy enough with string
manipulation.

• Bob

On May 8, 2014, at 12:15 AM, Daniel Lee notifications@github.com wrote:

If that's what's causing the confusion, we could reimplement that portion
of it, have the same behavior, but different message.

I'm not a fan of a warning message saying that there's an error. No wonder
it's causing confusion.

On Wed, May 7, 2014 at 6:07 PM, Michael Betancourt <notifications@github.com

wrote:

Yeah, I think Bob summarized it best: what happens is that the log_prob
function throws an error,
the sampler catches it and downgrades it to a warning but displays the
original error message
including the apparently indecipherable “Error in function” bit.

On May 7, 2014, at 10:54 PM, Daniel Lee notifications@github.com wrote:

I found out where the "Error in function:" comes from. It's in
boost/math/policies/error_handling.hpp, inside the raise_error function.

We already reimplemented most of the exception handling, so perhaps we
should bypass Boost's error handling altogether.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/640#issuecomment-42489702
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[betanalpha]

The offending code is

    try {
      stan::model::gradient(_model, z.q, z.V, z.g, _err_stream);
      z.V *= -1;
    } catch (const std::exception& e) {
      this->_write_error_msg(_err_stream, e);
      z.V = std::numeric_limits<double>::infinity();
    }

paired with

void _write_error_msg(std::ostream* error_msgs,
const std::exception& e) {

      if (!error_msgs) return;

      *error_msgs << std::endl
                  << "Informational Message: The current Metropolis proposal is about to be "
                  << "rejected becuase of the following issue:"
                  << std::endl
                  << e.what() << std::endl
                  << "If this warning occurs sporadically, such as for highly constrained "
                  << "variable types like covariance matrices, then the sampler is fine,"
                  << std::endl
                  << "but if this warning occurs often then your model may be either severely "
                  << "ill-conditioned or misspecified."
                  << std::endl;

  }

You’ll see that _write_error_msg prints e.what(), and it’s the original exception
that includes the “Error” phrase. What we’d want to do is go back to the constrained
variable exceptions and remove “Error” there.

On May 7, 2014, at 11:36 PM, Bob Carpenter notifications@github.com wrote:

How can we do that? The error gets thrown in the context
of the function evaluation, which doesn't know its context.
Catch and rethrow with a different message?

• Bob

On May 8, 2014, at 12:33 AM, Michael Betancourt notifications@github.com wrote:

We’re catching it but repeating the original error verbatim.
I think what we really want to do is separate the level of the
issue (warning/error) from the description (NaN, 0, etc).

On May 7, 2014, at 11:28 PM, Bob Carpenter notifications@github.com wrote:

If we catch all the errors, then it should be OK to turn
off the Boost error handling. But apparently we're not
catching this one ourselves if the error's coming from Boost.

A very crude fix would be to have the informational message
just remove the embedded "Error" and replace it with "Difficulty"
or something like that --- it's easy enough with string
manipulation.

• Bob

On May 8, 2014, at 12:15 AM, Daniel Lee notifications@github.com wrote:

If that's what's causing the confusion, we could reimplement that portion
of it, have the same behavior, but different message.

I'm not a fan of a warning message saying that there's an error. No wonder
it's causing confusion.

On Wed, May 7, 2014 at 6:07 PM, Michael Betancourt <notifications@github.com

wrote:

Yeah, I think Bob summarized it best: what happens is that the log_prob
function throws an error,
the sampler catches it and downgrades it to a warning but displays the
original error message
including the apparently indecipherable “Error in function” bit.

On May 7, 2014, at 10:54 PM, Daniel Lee notifications@github.com wrote:

I found out where the "Error in function:" comes from. It's in
boost/math/policies/error_handling.hpp, inside the raise_error function.

We already reimplemented most of the exception handling, so perhaps we
should bypass Boost's error handling altogether.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/640#issuecomment-42489702
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[syclik]

Going back to @betanalpha's comment, that e.what() is currently created by Boost's error handling. We can easily replace that.

Here's what the Boost function looks like:

template <class E, class T>
void raise_error(const char* function, const char* message)
{
  if(function == 0)
       function = "Unknown function operating on type %1%";
  if(message == 0)
       message = "Cause unknown";

  std::string msg("Error in function ");
  msg += (boost::format(function) % typeid(T).name()).str();
  msg += ": ";
  msg += message;

  E e(msg);
  boost::throw_exception(e);
}

Inside our raise_domain_error(), we just call that function. It would be really easy to replace that function.

Here is what an example message looks like:
Informational Message: The current Metropolis proposal is about to be rejected becuase of the following issue:
Error in function stan::prob::student_t_log(N4stan5agrad3varE): Scale parameter is 1.#INF:0, but must be finite!
If this warning occurs sporadically, such as for highly constrained variable types like covariance matrices, then the sampler is fine,
but if this warning occurs often then your model may be either severely ill-conditioned or misspecified.

_write_error_msg() is responsible for lines 1, 3, and 4.
check_finite() (called from student_t_log()), which is responsible for this part: "Scale parameter is 1.#INF:0, but must be finite!"
student_t_log() provides check_finite() with the string stan::prob::student_t_log(%1%).
Finally, boost::math::detail::raise_error() is responsible for "Error in function..." and putting the string together.

If we were to change the wording, what would be a reasonable change?

1. replace "Error in function" with "Exception in function"?
2. replace "Error in function" with "in function"?
3. remove "Error in function"?

I'm leaning towards 3 right now. Anyone want to weigh in? If we did that, could we leave the reporting as-is?

[betanalpha]

Personally I have no problem with error (it is an error in the underlying function)
but given the subtly between error in the function and the ability of the sampler
to downgrade that to a warning I understand the motivation to change it. I’d
go with (3) as well.

On May 8, 2014, at 3:01 PM, Daniel Lee notifications@github.com wrote:

Going back to @betanalpha's comment, that e.what() is currently created by Boost's error handling. We can easily replace that.

Here's what the Boost function looks like:

template <class E, class T>
void raise_error(const char* function, const char* message)
{
if(function == 0)
function = "Unknown function operating on type %1%";
if(message == 0)
message = "Cause unknown";

std::string msg("Error in function ");
msg += (boost::format(function) % typeid(T).name()).str();
msg += ": ";
msg += message;

E e(msg);
boost::throw_exception(e);
}
Inside our raise_domain_error(), we just call that function. It would be really easy to replace that function.

Here is what an example message looks like:
Informational Message: The current Metropolis proposal is about to be rejected becuase of the following issue:
Error in function stan::prob::student_t_log(N4stan5agrad3varE): Scale parameter is 1.#INF:0, but must be finite!
If this warning occurs sporadically, such as for highly constrained variable types like covariance matrices, then the sampler is fine,
but if this warning occurs often then your model may be either severely ill-conditioned or misspecified.

_write_error_msg() is responsible for lines 1, 3, and 4.
check_finite() (called from student_t_log()), which is responsible for this part: "Scale parameter is 1.#INF:0, but must be finite!"
student_t_log() provides check_finite() with the string stan::prob::student_t_log(%1%).
Finally, boost::math::detail::raise_error() is responsible for "Error in function..." and putting the string together.

If we were to change the wording, what would be a reasonable change?

1. replace "Error in function" with "Exception in function"?
2. replace "Error in function" with "in function"?
3. remove "Error in function"?

I'm leaning towards 3 right now. Anyone want to weigh in? If we did that, could we leave the reporting as-is?

—
Reply to this email directly or view it on GitHub.

[bob-carpenter]

I'd really like it if someone else took the fall this time.
I've already tried several variant wordings and none have worked.

Any wording that even hints of "error", including "exception", "warning",
etc. is going to cause the same issues with users. It has to be
very very clear that everything's OK.

If you say "if it only happens a few times", we'll be bombarded with
messages about what that means.

The reference to Metropolis isn't sinking in because I many of
users probably don't know much about MCMC. So I'd go with something
more minimal.

The manual needs to be changed as part of this. We should
probably add a chapter on errors and warnings --- that might
help at least with users who read or search the manual.

"error" can be included for initialization errors, etc.
that really are errors.

• Bob

P.S. Since you're wading into R, it'd be nice to have the messages
not tank the R GUI, but that's a separate issue.

On May 8, 2014, at 4:01 PM, Daniel Lee notifications@github.com wrote:

Going back to @betanalpha's comment, that e.what() is currently created by Boost's error handling. We can easily replace that.

Here's what the Boost function looks like:

template <class E, class T>
void raise_error(const char* function, const char* message)
{
if(function == 0)
function = "Unknown function operating on type %1%";
if(message == 0)
message = "Cause unknown";

std::string msg("Error in function ");
msg += (boost::format(function) % typeid(T).name()).str();
msg += ": ";
msg += message;

E e(msg);
boost::throw_exception(e);
}

Inside our raise_domain_error(), we just call that function. It would be really easy to replace that function.

Here is what an example message looks like:
Informational Message: The current Metropolis proposal is about to be rejected becuase of the following issue:
Error in function stan::prob::student_t_log(N4stan5agrad3varE): Scale parameter is 1.#INF:0, but must be finite!
If this warning occurs sporadically, such as for highly constrained variable types like covariance matrices, then the sampler is fine,
but if this warning occurs often then your model may be either severely ill-conditioned or misspecified.

_write_error_msg() is responsible for lines 1, 3, and 4.
check_finite() (called from student_t_log()), which is responsible for this part: "Scale parameter is 1.#INF:0, but must be finite!"
student_t_log() provides check_finite() with the string stan::prob::student_t_log(%1%).
Finally, boost::math::detail::raise_error() is responsible for "Error in function..." and putting the string together.

If we were to change the wording, what would be a reasonable change?

1. replace "Error in function" with "Exception in function"?
2. replace "Error in function" with "in function"?
3. remove "Error in function"?

I'm leaning towards 3 right now. Anyone want to weigh in? If we did that, could we leave the reporting as-is?

—
Reply to this email directly or view it on GitHub.

[syclik]

Just to recap, I think there are two different things to do regarding implementation:

• remove "error", "exception", "warning" from what we report #752 remove "error", "exception", "warning" from what we report
• reduce the frequency of metropolis rejection messages #753 reduce the frequency of the messages

I'll update this comment with the appropriate issue numbers once I create them.

[bob-carpenter]

• point users to manual section to describe error

I actually don't think we should reduce the frequency of the messages.

[mbrubake]

I really think we should include:

• Include model line number in warnings, errors and rejection messages.
  If users are better able to see why they're getting the error message, they are much better positioned to understand it and how to fix it.

[bob-carpenter]

I think that'd be great. I see two ways of doing it,
both of which are going to require substantial effort
to code

1. Add a new argument to every function call with more
   info, like line number.
2. Instrument the generated code to increment the line
   number and then print when we get an exception.

Seems like (2) would be faster in terms of the execution
time and perhaps easier to build.

Anybody see any other way to do this?

On Jul 9, 2014, at 5:16 PM, Marcus Brubaker notifications@github.com wrote:

I really think we should include:

• Include model line number in warnings, errors and rejection messages. If users are better able to see why they're getting the error message, they are much better positioned to understand it and how to fix it.
—
Reply to this email directly or view it on GitHub.

[mbrubake]

I seem to recall us having a discussion about this on stan-dev several
months ago. I think the winning idea was to wrap all the generated code
with a try/catch block. Before each statement a variable would be set with
the line number of what was about to be called which could then be
referenced if an exception was thrown. The catch block would take the
exception, add the line number and maybe the actual line(s) to the
exception message and rethrow it. E.G.,

unsigned int line_num;

try {
  line_num = 1;
  x = A*b;

  line_num = 2;
  lp__ += normal_log(x,mu,Sigma);
}
catch(std::exception &e) {
  e.message = "Exception thrown on line number " + line_num +": \n" +
code[line_num] + e.message;
  rethrow e;
}

or something like that.

The issue with this has always been getting the model line number from the
parser to the code generator.

On Wed, Jul 9, 2014 at 11:19 PM, Bob Carpenter notifications@github.com
wrote:

I think that'd be great. I see two ways of doing it,
both of which are going to require substantial effort
to code

1. Add a new argument to every function call with more
   info, like line number.
2. Instrument the generated code to increment the line
   number and then print when we get an exception.

Seems like (2) would be faster in terms of the execution
time and perhaps easier to build.

Anybody see any other way to do this?

On Jul 9, 2014, at 5:16 PM, Marcus Brubaker notifications@github.com
wrote:

I really think we should include:

• Include model line number in warnings, errors and rejection messages.
If users are better able to see why they're getting the error message, they
are much better positioned to understand it and how to fix it.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#640 (comment).

[bob-carpenter]

Exactly --- that was proposal 2.

I think I can get the line numbers with the new iterators.
I'll have to see.

This is definitely worth doing, but it's not going to happen
by 2.4.

• Bob

On Jul 9, 2014, at 11:37 PM, Marcus Brubaker notifications@github.com wrote:

I seem to recall us having a discussion about this on stan-dev several
months ago. I think the winning idea was to wrap all the generated code
with a try/catch block. Before each statement a variable would be set with
the line number of what was about to be called which could then be
referenced if an exception was thrown. The catch block would take the
exception, add the line number and maybe the actual line(s) to the
exception message and rethrow it. E.G.,

unsigned int line_num; 

try { 
line_num = 1; 
x = A*b; 

line_num = 2; 
lp__ += normal_log(x,mu,Sigma); 
} 
catch(std::exception &e) { 
e.message = "Exception thrown on line number " + line_num +": \n" + 
code[line_num] + e.message; 
rethrow e; 
} 

or something like that.

The issue with this has always been getting the model line number from the
parser to the code generator.

On Wed, Jul 9, 2014 at 11:19 PM, Bob Carpenter notifications@github.com
wrote:

I think that'd be great. I see two ways of doing it,
both of which are going to require substantial effort
to code

1. Add a new argument to every function call with more
   info, like line number.
2. Instrument the generated code to increment the line
   number and then print when we get an exception.

Seems like (2) would be faster in terms of the execution
time and perhaps easier to build.

Anybody see any other way to do this?

On Jul 9, 2014, at 5:16 PM, Marcus Brubaker notifications@github.com
wrote:

I really think we should include:

• Include model line number in warnings, errors and rejection messages.
If users are better able to see why they're getting the error message, they
are much better positioned to understand it and how to fix it.
—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#640 (comment).

—
Reply to this email directly or view it on GitHub.