Distribution Testing

[Peter Li]

So far, I've been able to modify the generate_tests so it generates all of the tests, though there are repeats--I can't seem to find a way to generate the test cases without repeating everything including doubles (i.e. normal_log(double,double,double) is repeated four extra times). The issue here is that I can't just throw everything into the argument list when I generate the tests since we can't have something like normal_log(fvar<double>, var, double) so I generated everything with some variation of fvar<double>,var,fvar<fvar<double>>,fvar<var>,fvar<fvar<var>> separately. 

The main issue I'm having right now is that when testing gradients and finite diffs, the code is different for fvar<var>,var,fvar<double>, fvar<fvar<double>>, and fvar<fvar<var>> (not to mention we want to test higher orders for the fvar<fvar< > > ones). So we could have all of this in one file which would probably be quite messy with all the enable_ifs or specializations or we can have 5 different files (one for fvar<double>, one for fvar<var>, one for fvar<fvar<double>>, one for fvar<fvar<var>>, and one for var). If we go with the latter (which I think would probably be easier to implement), that would mean that we would have some duplicate code (namely, all of the tests that don't involve gradients). 

Thoughts?

[Bob Carpenter]

I would very much like to test through functionals that
will work no matter how we implement autodiff. That way,
as we optimize it (or even rewrite it) over the next year,
we'll have tests in place.

See agrad/autodiff.hpp

You then need to figure out how to make a functor out of
a density that you can pass into the derivative functionals.

- 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.

[Peter Li]

I'm confused. Are you saying to use the code in autodiff.hpp to test the distributions?

[Ben Goodrich]

On Tuesday, June 10, 2014 9:51:18 PM UTC-4, Peter Li wrote:

Thoughts

This does not really respond to Peter's question, but I think the current distribution tests are overkill. They take forever and don't catch bugs very efficiently.

Currently, we are testing all combinations var and double (possibly in vectors) for all of the arguments, and I guess soon for the various fvar types. I think we could get away with just testing that each distribution works when all of its arguments are simultaneously of type T.

This would not catch bugs where, in the normal distribution example, y is a double vector, mu is a var vector, and sigma is a double, etc. But all those combinations are really just exercising the type promotion stuff, which is something that could be (and hopefully is) done more directly with unit tests. And I think after this release comes out, we should switch to C++11 and be using auto internally inside the distributions, so that the scope for such bugs is a lot narrower.

Maybe it is worthwhile to make sure the compiler is instantiating all the various combinations of arguments correctly, but that could be done much more quickly by putting those in the -fsyntax-only pile.

Ben

[Daniel Lee]

On Wed, Jun 11, 2014 at 11:47 AM, Ben Goodrich <goodri...@gmail.com> wrote:

This does not really respond to Peter's question, but I think the current distribution tests are overkill. They take forever and don't catch bugs very efficiently.

Sorry, Ben, but I'm not going to budge on testing our distributions.

1. They're not overkill. They're actually underkill. There are ways bugs can still get through.

2. They take a long time, but it isn't forever. If you're talking about catching gradient bugs inefficiently, I've had to hunt down bugs in gradients of our distributions by creating a model, running it, comparing the model distribution to what we expect it to be, figuring out which distributions it calls, finding out the parameter values that get passed into each distribution, making sure those all work, just to identify which piece has a bug. To me, that's much more inefficient. By the way, to test a single distribution family, including all the compilation time, it takes under 2 mins. I tested the normal distribution.

3. It may be overkill to run it on each commit, although I don't think so. Unfortunately, testing traits and other templated code might have weird effects that we can't effectively "unit" test. Or fail to somehow. That really means we should be testing it somewhere else, but they're things that we're not even aware we should be testing.

4. These tests have been really effective in stopping defective vectorized distributions. I don't think you, Ben, have written any, so I'm not sure you're aware of the complexity of the way we're writing the distributions now. When we used to use auto-diff directly, even over vector arguments, life was a lot easier and we really didn't need this type of testing. Now, with all the optimizations we've taken with the distributions, these test make it a lot easier and quicker to get out more correct code.

I'm not saying that this is the best way to do it. If you can come up with something better, please, implement it or let someone know.

 

Currently, we are testing all combinations var and double (possibly in vectors) for all of the arguments, and I guess soon for the various fvar types. I think we could get away with just testing that each distribution works when all of its arguments are simultaneously of type T.

Yes, we're testing every combination of var, double x scalar, vector, row_vector, array. We can't get away with just a single type of T. More below.

 

This would not catch bugs where, in the normal distribution example, y is a double vector, mu is a var vector, and sigma is a double, etc. But all those combinations are really just exercising the type promotion stuff, which is something that could be (and hopefully is) done more directly with unit tests. And I think after this release comes out, we should switch to C++11 and be using auto internally inside the distributions, so that the scope for such bugs is a lot narrower.

So, this is where you're missing the point of this testing. It isn't exercising the type promotion. We're actually doing things at the double level, for these distributions.

Suppose there's a 2 argument probability distribution. If the first is scalar and the second is scalar, that's easy. If they're both vectors, that's easy too. The gradients are as expected. If the first is scalar and the second is a vector, the expression graph is going to put the first on the expression graph multiple times and the derivative with respect to that first variable is going to have to be adjusted. Same with the second variable.

We've "caught" a lot of bugs using the framework. It's really easy, as a programmer, to get indexes mixed up and increment the wrong derivative. It's easy to forget to loop over both variables in the same way. It's easy to forget to compute a vector of things if you needed them. These things should be straight-forward, and were at one point before we started optimizing our distribution code. To do what we're doing, I'd argue we need something like our current framework or better.

Maybe it is worthwhile to make sure the compiler is instantiating all the various combinations of arguments correctly, but that could be done much more quickly by putting those in the -fsyntax-only pile.

Ben

Sorry, but I'm adamant about keeping this level of testing as a minimum for the distribution tests. If you know of a faster way to do it, please let us know. Just FYI, we test:

- all versions of the function are callable (this is the instantiation that you're talking about)

- values, specified by the test writer, are callable

- invalid values, specified by the test writer, throw exceptions

- make sure the proportionality constant is actually constant

- we test against finite diff, just in case we messed up our hand calculations

- we test against a second implementation, usually pure auto-diff, specified in the test itself

- we test with each value repeated in a vector. We make sure the log prob value is a multiple of the first, we make sure the gradient value is a multiple when it's a scalar and the other arguments have been broadcast.

- we test with a 0 length vector

I think it's an ugly testing framework, but does the job, for now. I'd be happy to replace it.

 

[Ben Goodrich]

On Wednesday, June 11, 2014 1:23:01 PM UTC-4, Daniel Lee wrote:

On Wed, Jun 11, 2014 at 11:47 AM, Ben Goodrich <goodri...@gmail.com> wrote:

This does not really respond to Peter's question, but I think the current distribution tests are overkill. They take forever and don't catch bugs very efficiently.

Sorry, Ben, but I'm not going to budge on testing our distributions.

I guess I wasn't clear that by T I meant any type of thing that can be passed to one of the distributions. So, we definitely need to test all possible homogenous arguments like

• double, double, ...
• var, var, ...
• fvar<double>, fvar<double>, ...
• fvar<var>, fvar<var>, ...
  
• (row)vector of double, (row)vector of double, ...
• (row)vector of var, (row)vector of var, ...
• (row)vector of fvar<double>, (row)vector of fvar<double>, ...
• (row)vector of fvar<var>, (row)vector of fvar<var>, ...
  

1. They're not overkill. They're actually underkill. There are ways bugs can still get through.

2. They take a long time, but it isn't forever. If you're talking about catching gradient bugs inefficiently, I've had to hunt down bugs in gradients of our distributions by creating a model, running it, comparing the model distribution to what we expect it to be, figuring out which distributions it calls, finding out the parameter values that get passed into each distribution, making sure those all work, just to identify which piece has a bug. To me, that's much more inefficient. By the way, to test a single distribution family, including all the compilation time, it takes under 2 mins. I tested the normal distribution.

The normal is not so bad because it only has 3 arguments; exp_mod_normal and stuff are a lot worse. And these tests are a pretty big barrier for implementing distributions with 5 or 6 arguments where MCMC or MML should have a big advantage over ML. But in the plain normal case, what sort of gradient bug would be caught by testing {double, double, var}, {double, var, double}, {double, var, var}, {var, double, double}, {var, var, double}, {var, double, var}, that wouldn't be caught just by testing {var, var, var}?
 

3. It may be overkill to run it on each commit, although I don't think so. Unfortunately, testing traits and other templated code might have weird effects that we can't effectively "unit" test. Or fail to somehow. That really means we should be testing it somewhere else, but they're things that we're not even aware we should be testing.

I think that implies we're doing it wrong.

4. These tests have been really effective in stopping defective vectorized distributions. I don't think you, Ben, have written any, so I'm not sure you're aware of the complexity of the way we're writing the distributions now. When we used to use auto-diff directly, even over vector arguments, life was a lot easier and we really didn't need this type of testing. Now, with all the optimizations we've taken with the distributions, these test make it a lot easier and quicker to get out more correct code.

I'm not saying that this is the best way to do it. If you can come up with something better, please, implement it or let someone know.

See the end, but also to replace as much of the type promotion stuff as possible with auto and decltype.
 

Currently, we are testing all combinations var and double (possibly in vectors) for all of the arguments, and I guess soon for the various fvar types. I think we could get away with just testing that each distribution works when all of its arguments are simultaneously of type T.

Yes, we're testing every combination of var, double x scalar, vector, row_vector, array. We can't get away with just a single type of T. More below.

 

This would not catch bugs where, in the normal distribution example, y is a double vector, mu is a var vector, and sigma is a double, etc. But all those combinations are really just exercising the type promotion stuff, which is something that could be (and hopefully is) done more directly with unit tests. And I think after this release comes out, we should switch to C++11 and be using auto internally inside the distributions, so that the scope for such bugs is a lot narrower.

So, this is where you're missing the point of this testing. It isn't exercising the type promotion. We're actually doing things at the double level, for these distributions.

Suppose there's a 2 argument probability distribution. If the first is scalar and the second is scalar, that's easy. If they're both vectors, that's easy too. The gradients are as expected. If the first is scalar and the second is a vector, the expression graph is going to put the first on the expression graph multiple times and the derivative with respect to that first variable is going to have to be adjusted. Same with the second variable.

I get that. To me, that is an area where we should (if we don't already) have unit-tests of the OperandsAndPartials and ScalarTypeVectorView and whatnot. We definitely need to make sure that the expression graph has the right number of leaves and stuff for all possible inputs. But given that, I don't see why we need to re-verify that for all distributions that utilize OperandsAndPartials and ScalarTypeVectorView because the structure of the expression graph doesn't depend on the particularities of the partial derivatives.
 

We've "caught" a lot of bugs using the framework. It's really easy, as a programmer, to get indexes mixed up and increment the wrong derivative. It's easy to forget to loop over both variables in the same way. It's easy to forget to compute a vector of things if you needed them. These things should be straight-forward, and were at one point before we started optimizing our distribution code. To do what we're doing, I'd argue we need something like our current framework or better.

It seems to me that off-by-one and other indexing errors would be caught with a homogenous test of {vector<var>, vector<var>, ...}.
 

Sorry, but I'm adamant about keeping this level of testing as a minimum for the distribution tests. If you know of a faster way to do it, please let us know. Just FYI, we test:

- all versions of the function are callable (this is the instantiation that you're talking about)

okay, that part is amenable to -fsyntax-only
 

- values, specified by the test writer, are callable

not sure what you mean by that
 

- invalid values, specified by the test writer, throw exceptions

if the invalid value throws an exception for homogenous invalid argument types, then it should throw an exception for mixed invalid argument types
 

- make sure the proportionality constant is actually constant

I think this could be verified with homogenous double argument types.
 

- we test against finite diff, just in case we messed up our hand calculations

- we test against a second implementation, usually pure auto-diff, specified in the test itself

I think these could be verified with all homogenous not-double argument types, like {var, var, ...}, {vector<var>, vector<var>, ...}, {fvar<double>, fvar<double>, ...}, etc.
 

- we test with each value repeated in a vector. We make sure the log prob value is a multiple of the first, we make sure the gradient value is a multiple when it's a scalar and the other arguments have been broadcast.

If I am understanding you correctly, I think that could be a unit-test rather than a bunch of distribution-specific tests. In other words, whether we get that right doesn't have to do with what value the partial derivative takes.
 

- we test with a 0 length vector

Just need a homogenous call with {vector<double>, vector<double>, ...} where the vectors are each length zero. We don't need to also do {vector<var>, vector<var>, ...} there.

Ben

[Bob Carpenter]

On Jun 11, 2014, at 4:29 PM, Peter Li <li.pe...@gmail.com> wrote:

> I'm confused. Are you saying to use the code in autodiff.hpp to test the distributions?
>

Exactly. Then the tests shouldn't need to involve the
actual types fvar, var, etc.

And you can write finite differences functionals like the
ones in autodiff.hpp that can also be used for testing.

The issue is that we're planning to rebuild the auto-diff to
be much faster, but have different underlying behavior and types,
and it would be nice not to have to rewrite all the tests.

- Bob

[Bob Carpenter]

On Jun 11, 2014, at 8:24 PM, Ben Goodrich <goodri...@gmail.com> wrote:

> But in the plain normal case, what sort of gradient bug would be caught by testing {double, double, var}, {double, var, double}, {double, var, var}, {var, double, double}, {var, var, double}, {var, double, var}, that wouldn't be caught just by testing {var, var, var}?

That's the crux of the matter. If you look at the code, template
traits are used to select branches in the code based on whether the
argument is a var or a double. If it's double, you drop the term.

Also, the vector vs. var invokes all the expression template view machinery.

>
> 3. It may be overkill to run it on each commit, although I don't think so. Unfortunately, testing traits and other templated code might have weird effects that we can't effectively "unit" test. Or fail to somehow. That really means we should be testing it somewhere else, but they're things that we're not even aware we should be testing.
>
> I think that implies we're doing it wrong.

Doing what wrong?

> It seems to me that off-by-one and other indexing errors would be caught with a homogenous test of {vector<var>, vector<var>, ...}.

Not quite, because there are different branches in the code
in many cases. The point that Ben's made before and I'll second
is that we want to test coverage. But it's not just coverage,
it's the combinatorics of how the evaluations can go.

Like Daniel, I can't see any way of getting around this.

> - invalid values, specified by the test writer, throw exceptions
>
> if the invalid value throws an exception for homogenous invalid argument types, then it should throw an exception for mixed invalid argument types

I think that's right.

> - make sure the proportionality constant is actually constant
>
> I think this could be verified with homogenous double argument types.

Not quite --- see above.

> - we test against finite diff, just in case we messed up our hand calculations

This has saved us on many occasions.

> - we test against a second implementation, usually pure auto-diff, specified in the test itself

I'm not sure we need both of these.

> - we test with each value repeated in a vector. We make sure the log prob value is a multiple of the first, we make sure the gradient value is a multiple when it's a scalar and the other arguments have been broadcast.
>
> If I am understanding you correctly, I think that could be a unit-test rather than a bunch of distribution-specific tests. In other words, whether we get that right doesn't have to do with what value the partial derivative takes.

I don't quite understand what you're suggesting here.

- Bob

[Daniel Lee]

I just wanted to mention, the runtime of the tests is very short. It's almost all in compilation time.

Just timing on my machine: total time to compile and run the normal distribution tests (24 of them), as reported by time:
real    1m49.751s
user    9m34.293s
sys    0m26.228s

The runtime (recorded by google test)

 1.681s

So, it's really not the runtime of the tests that matter. We need to get a faster build.

[Peter Li]

The only issue I see is that the tests in autodiff are not vectorized. That's not an issue except for when we do test_repeat_as_vector() when parameters are repeated as vectors and then passed in. Also, it seems like you can't have a function and use inputs something like double double fvar<double>.

I'm also confused at how I would use the stuff in autodiff.hpp to calculate higher order derivatives (namely second and third) since they don't use the var call .grad at all. [If you want to see what I'm referring to look in the branch at test_fixture_distr.hpp]

Let me know if there are any workarounds for these. In any case, I think the way I've got most of testing implemented now, even if we change the autodiff system, it would only change types and the small functions where we compute derivatives like 

  double calculate_gradients_3rdorder(vector<double>& grad,

                                      fvar<fvar<var> >& logprob, 

                                      vector<var>& x) {

    logprob.d_.d_.grad(x, grad);

    return logprob.val_.val_.val();

  }

[Michael Betancourt]

See the bottom of https://github.com/stan-dev/stan/blob/develop/src/stan/model/util.hpp.

I’ll be adding tests for the new stuff from the ground up as well.

[Bob Carpenter]

On Jun 12, 2014, at 5:18 PM, Peter Li <li.pe...@gmail.com> wrote:

> The only issue I see is that the tests in autodiff are not vectorized. That's not an issue except for when we do test_repeat_as_vector() when parameters are repeated as vectors and then passed in. Also, it seems like you can't have a function and use inputs something like double double fvar<double>.

You need to define the appropriate functor. It may not
be worth doing.

> I'm also confused at how I would use the stuff in autodiff.hpp to calculate higher order derivatives (namely second and third) since they don't use the var call .grad at all. [If you want to see what I'm referring to look in the branch at test_fixture_distr.hpp]

There's a Hessian function for 2nd order and you can add a 3rd order
version to autodiff.hpp.

> Let me know if there are any workarounds for these. In any case, I think the way I've got most of testing implemented now, even if we change the autodiff system, it would only change types and the small functions where we compute derivatives like
>
> double calculate_gradients_3rdorder(vector<double>& grad,
> fvar<fvar<var> >& logprob,
> vector<var>& x) {
> logprob.d_.d_.grad(x, grad);
> return logprob.val_.val_.val();
> }

I'm not following the types here. First of all, where's the function
that's being evaluated?

In general, I think we ONLY need to define and test inputs where
every type is either the highest type or double/int. We won't
be calling things like normal_log(fvar<var>, var, double)
or anything like that.

- Bob

[Peter Li]

This particular one is for fvar<fvar<var> > types. x is the vector of all the vars that we are taking the gradient to. So for all the parameters that are of type fvar<fvar<var> > i added param[i].val_.val_ to the vector x. And then we're taking the gradient of logprob with respect to these vars to get the resulting vector grad which will consist of doubles. I defined functions like these for all the types. I think that with the new autodiff, most of what we need to change should be mostly only these functions.

- Bob

[Peter Li]

I've added higher order finite-diff tests, but the issue is that when the types are something like fvar<var>,fvar<var>,double, the derivative of logp will be something like (dlogp/dx1) *dx1_ + (dlogp/dx2)*dx2_, so then when we take the gradient of this, what we get isn't actually the partial derivatives d^2/dx1^2 and d^2/dx2^2. Instead, it's a combination of what we want and d^2/dx1dx2. 

In any case, it might be difficult to test this, but first order always works and I think since calculating higher order partial derivatives just reuses the first order code, we might be ok since the individual functions have higher order partial derivative tests (which I'm still working on adding for some matrix operations).