Skip to content
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

Feature/0557 fvar test kit #562

Merged
merged 15 commits into from Jun 28, 2017
Merged

Feature/0557 fvar test kit #562

merged 15 commits into from Jun 28, 2017

Conversation

@bob-carpenter
Copy link
Contributor

bob-carpenter commented Jun 10, 2017

Submission Checklist

  • Run unit tests: ./runTests.py test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary:

First pull request for #557 showing how the framework will apply to binary functions with scalar returns. The reason this is being done as a separate pull request is that there will be a lot of files being removed and I wanted to lay out the general pattern.

Intended Effect:

Remove hundreds of lines of test code per function and make tests more thorough and easier to write for new functions.

How to Verify:

Please look at the implementation and, of course, the tests.

Side Effects:

No.

Documentation:

Added some doc for the test framework.

Reviewer Suggestions:

@seantalts, @betanalpha, @syclik

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):

Columbia University

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 11, 2017

@syclik and @seantalts --- no idea how it's looking for dependencies, but it found a mention of stan::math::var in the comments and balked. No idea if that was intended, but it surprised me. I'll just have to leave the use of mixed mode undocumented as there isn't a file to put it in. I went with words rather than code instead to get this to pass.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 14, 2017

Jenkins, retest this please

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 24, 2017

I'm perplexed as to why I'm getting failures upstream on a logistic test. So far, this pull request hasn't changed any behavior, just added a testing framework for operator+. I'm going to kick off tests again.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 24, 2017

Jenkins, retest this please

@seantalts

This comment has been minimized.

Copy link
Member

seantalts commented Jun 24, 2017

…re/0557-fvar-test-kit
@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 24, 2017

Thanks, @seantalts. I indeed hadn't merged in a while. Looks like all the tests are kicking off again the right way.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 25, 2017

@seantalts Is jenkins hung up again? This hasn't kicked off testing.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 25, 2017

Never mind---I see another one running and will check in again when that's done. I think we're hitting our load limit with testing.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 26, 2017

This one's now passing and just needs a review. I'll get working on pushing this through the whole function lib in another pull request---I just wanted to get the basic framework vetted first.

@seantalts

This comment has been minimized.

Copy link
Member

seantalts commented Jun 26, 2017

Mind if I review this one?

@syclik

This comment has been minimized.

Copy link
Member

syclik commented Jun 26, 2017

@syclik

This comment has been minimized.

Copy link
Member

syclik commented Jun 26, 2017

@bob-carpenter, this is missing a test_throw or equivalent, as outlined in the original issue. For the example provided, it shouldn't throw, but want to provide the function as part of this pull request?

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 26, 2017

@syclik --- You mean test a second function that throws to make sure the throws test works?

I'm also not testing the array writer pattern for all the different types of inputs.

Would you like me to set up a half dozen or so mock functions and test those to cover more of the combinatorics of input types and exception throwing?

Also, any idea how to test throwing different types of exceptions in a general framework? Almost all of our functions only throw a handful of error types, but the only way I see how to do this is collect instances of all of them in separate containers. I just don't see how to get type info in otherwise.

@syclik

This comment has been minimized.

Copy link
Member

syclik commented Jun 26, 2017

@syclik --- You mean test a second function that throws to make sure the throws test works?

No, just the part of the testing framework that tests for exceptions. It was outlined in the original issue. I just didn't find it in the implementation here.

From the original issue #557:

t.throw(NaN, 1.0, std::domain_error);

If you just want to hold off until we implement the test for a function that throws, that's fine.

I'm also not testing the array writer pattern for all the different types of inputs.

Would you like me to set up a half dozen or so mock functions and test those to cover more of the combinatorics of input types and exception throwing?

Nope. No need. No need to test the test code.... seems clear enough and if there are problems, we'll fix them as they come up.

Also, any idea how to test throwing different types of exceptions in a general framework? Almost all of our functions only throw a handful of error types, but the only way I see how to do this is collect instances of all of them in separate containers. I just don't see how to get type info in otherwise.

No wonder Google Test uses macros. Would you be offended by a hybrid macro solution? If not, I can see us having things like test_throws_runtime_error and test_throws_invalid_argument.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor Author

bob-carpenter commented Jun 26, 2017

Ah, never mind on that last point. It's been so long since this went into a pull request I forgot that I rewrote it to do a single test at a time. For that, I can specify the expected expectation type as a template parameter.

Personally, I'd rather stay away from macros and code generation to the extent possible.

Copy link
Member

seantalts left a comment

I think this is good! I had some comments that you may want to look at but I think the change is good.

// size 0 separate because nothing to loop over in main body
if (x.size() == 0) {
fx = f(x);
return;

This comment has been minimized.

Copy link
@seantalts

seantalts Jun 26, 2017

Member

I realize that you just copied this from another file, so maybe this isn't the right place to address this. But does this return uninitialized matrices for grad and H? Like shouldn't we set them to 0 instead of random memory contents even when x.size() == 0?

This comment has been minimized.

Copy link
@bob-carpenter

bob-carpenter Jun 28, 2017

Author Contributor

The matrix for Hessian and vector for gradient have to be matrices and vectors coming in because they're rerences. if x.size() == 0, then the function resizes the Hessian to be zero by zero and the gradient to be size zero on the previous lines. That will make sure everything's the right size and fully initialized (size zero objects don't need any values set).

f.fixed1_ = true;
f.fixed2_ = true;
seq_writer<double> a;
Eigen::VectorXd aaa(0);

This comment has been minimized.

Copy link
@seantalts

seantalts Jun 26, 2017

Member

Where is aaa used?

This comment has been minimized.

Copy link
@bob-carpenter

bob-carpenter Jun 28, 2017

Author Contributor

It's not. Removed it.

}

template <int R, int C>
matrix_t read(const Eigen::Matrix<double, R, C>& x) {

This comment has been minimized.

Copy link
@seantalts

seantalts Jun 26, 2017

Member

I realize these vectorized versions of read aren't used anywhere yet but it might be worth noting in the docstring maybe here and maybe in things that use binary_binder that the x being read into must already have its size specified before the call.

This comment has been minimized.

Copy link
@bob-carpenter

bob-carpenter Jun 28, 2017

Author Contributor

The return type there was wrong and should have been Matrix<T, R, C>. The only thing x is used for is its size. Nothing gets read into x here---the reader fills in a new matrix type object and returns it.

struct seq_writer {
std::vector<T> data_;


This comment has been minimized.

Copy link
@seantalts

seantalts Jun 26, 2017

Member

extra spaces

This comment has been minimized.

Copy link
@bob-carpenter

bob-carpenter Jun 28, 2017

Author Contributor

Fixed.

z.d_.d_.grad(p,g);
EXPECT_FLOAT_EQ(0, g[0]);
}
TEST(foo, framework) {

This comment has been minimized.

Copy link
@syclik

syclik Jun 27, 2017

Member

Want to changefoo to something like math? We have these legacy names which were can shed, but maybe with something more descriptive than a placeholder.

This comment has been minimized.

Copy link
@bob-carpenter

bob-carpenter Jun 28, 2017

Author Contributor

went with (mathMixCore, operatorPlus)

@syclik

This comment has been minimized.

Copy link
Member

syclik commented Jun 28, 2017

Thanks, Bob!

@bob-carpenter bob-carpenter merged commit a117992 into develop Jun 28, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@bob-carpenter bob-carpenter deleted the feature/0557-fvar-test-kit branch Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.