error handling is tangled with traits

[Daniel Lee]

Hi all,

So, I now understand why we have duplicate error checking code.

The original stuff was in:

src/stan/math/error_handling/check_*
src/stan/math/error_handling/matrix/check_*

New stuff was implemented in:
src/stan/math/validate_*
src/stan/math/matrix/validate_*

I'm finding out that the order of header includes for the check_* functions matters a lot. This shouldn't be the case, but it's true now. What ends up happening is that things stop compiling by bringing in an additional header.

I'm going to suggest we reimplement all of our error handling. (By we, I mean I am volunteering and will take anyone else's time if you've got it.) I want to address:
- issue #665 - consolidating error handling code: https://github.com/stan-dev/stan/issues/655

- issue #640 - eliminate error message when sampling

- untangling this header mess

- simplify error handling by decoupling our error handling from Boost's complex, policy-based error handling.

Fortunately, all the original error handling code has unit tests, so it's easy to validate that the new error handling is the same. Unfortunately, none of the new error handling has any tests, so it'll take a little more time to implement safely than if it all had tests in place.

The biggest design change will be to decouple from Boost's style of error handling. I don't think this is an issue, but here's a list of pros and cons:

Pros:

- much simpler code

- easier to include a single error handling function

- removes the "Error in:" message that Boost provides

- we'd have much more control over error messages

- it'll be a lot easier to break apart our auto-diff library in the future if this section isn't tied into the traits.

Cons:

- no ability to flexibility change behavior, at least for the first incarnation (Note: we don't EVER use this functionality)

Anyone oppose? Anyone want to help? Time estimate -- if I start now, good chance I knock this out by tomorrow's meeting. ~30 existing error check functions with tests, which should take a handful of hours (hopefully under 5 hours). ~15 new functions without tests -- this will eat up a lot of time. Yesterday, I spent about 8 hours moving 3 of them to our existing error check functions. (Anyone that thinks that unit tests don't matter for "trivial" bits of code, they do. It helps make changes like this a lot easier to do.)

Daniel

[Bob Carpenter]

I'm all for consolidating functionality.

So are you going to get rid of all the Boost config? I think
that's really no big deal from our perspective. It's not like
we're trying to build a Boost module. We originally thought it
might be nice to allow users to turn off exceptions for inclusions
in cases where code doesn't allow exceptions.

- Bob

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Daniel Lee]

On Wed, May 21, 2014 at 12:09 PM, Bob Carpenter <ca...@alias-i.com> wrote:

I'm all for consolidating functionality.

So are you going to get rid of all the Boost config?  

Yes. We don't really use the configuration anywhere.

 

I think that's really no big deal from our perspective.  It's not like
we're trying to build a Boost module.  We originally thought it
might be nice to allow users to turn off exceptions for inclusions
in cases where code doesn't allow exceptions.

I'll make sure we call a function to throw the exception instead of throw exceptions directly from the check functions. That'll let us be flexible in the future.

[Bob Carpenter]

On May 21, 2014, at 6:18 PM, Daniel Lee <bea...@alum.mit.edu> wrote:

> On Wed, May 21, 2014 at 12:09 PM, Bob Carpenter <ca...@alias-i.com> wrote:
> I'm all for consolidating functionality.
>
> So are you going to get rid of all the Boost config?
>
> Yes. We don't really use the configuration anywhere.

Other than in the gazillion unit tests. But the idea was always to
let someone else configure that if they wanted to, hence the testing.

> I think that's really no big deal from our perspective. It's not like
> we're trying to build a Boost module. We originally thought it
> might be nice to allow users to turn off exceptions for inclusions
> in cases where code doesn't allow exceptions.
>
> I'll make sure we call a function to throw the exception instead of throw exceptions directly from the check functions. That'll let us be flexible in the future.

Great. I think that's all we'll need.

- Bob

[Ben Goodrich]

On Wednesday, May 21, 2014 11:47:48 AM UTC-4, Daniel Lee wrote:

Cons:

- no ability to flexibility change behavior, at least for the first incarnation (Note: we don't EVER use this functionality)

If you are digging everything up anyway, can you make it such that if NDEBUG is defined, it turns off all non-essential error checking?

Ben

[Daniel Lee]

I'm going to suggest doing this in two stages. Let me get through and make sure we leave current functionality the same.

Then we can do what you suggest. A couple things:

1. Can you give guidance on how to do that? Is that preprocessor macros? Or something else?

2. How do we determine what's essential and what's not? I was hoping we could provide a template parameter down from the caller, but we don't have that in place now.

 

[Ben Goodrich]

On Wednesday, May 21, 2014 12:57:17 PM UTC-4, Daniel Lee wrote:

OK. I think you can just make every such function like this

bool check_something(...) {
  #ifdef NDEBUG
  check for something
  #endif
}

and the compiler will get rid of all the dead code if NDEBUG is defined. I'm not sure if anything is so essential that it must be checked unconditionally. We should definitely be willing to segfault if there is a problem with the model and NDEBUG is defined. No throwing exceptions, etc.

Ben

[Bob Carpenter]

On May 23, 2014, at 5:18 AM, Ben Goodrich <goodri...@gmail.com> wrote:

> ... I'm not sure if anything is so essential that it must be checked unconditionally. We should definitely be willing to segfault if there is a problem with the model and NDEBUG is defined. No throwing exceptions, etc.

As long as we plaster open-wheel racing mode (nitro drag race
mode for you Americans) with sufficient warnings and make clients
sign a waiver (modal dialog box for interactive embedded or
cluster use), I'd be OK with that.

Personally, I don't think I would ever run in such a mode unless
there were huge performance gains (more than a factor of two, say)
and I really trusted the model.

I do think there's utility in building a C++ API mode that fails quietly
without throwing exceptions for embedding. I think Daniel has that
covered in the design. We're pretty much only using the exceptions to
propagate to the point where we do a rejection.

- Bob

[Marcus Brubaker]

Actually, the ability to do this can have really big gains in some models.  For instance, the CAR models that use the matrix_normal_prec distribution.  Being able to turn off the SPD checks means we don't have to actually compute the Cholesky factorization of a fairly large, sparse (and constant) matrix.  In fact there was a user trying to run these models and Stan was intolerably slow so I had at one point made a special branch for him which turned off these checks.

Cheers,

Marcus