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

Add coverage checking to the tests. #224

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

j-towns
Copy link
Collaborator

@j-towns j-towns commented May 10, 2017

Firstly, you'll now need to have the coverage module installed in order to run the tests.

Running

nosetests tests

will now automatically generate a file .coverage in the root directory of the repo, containing coverage data.

To compile this file into user friendly html, run

coverage html

from the root directory.

I've also added a flag variable to autograd/util.py to enable more comprehensive coverage checking.

Running
nosetests tests
will now automatically generate a file .coverage in the root directory
of the repo, containing coverage data.

To compile this file into user friendly html, run
coverage html
from the root directory.

Also added a flag to autograd/util.py to enable more comprehensive
coverage checking.
@j-towns
Copy link
Collaborator Author

j-towns commented May 10, 2017

I'm thinking about doing another 'spring cleaning' pr tidying up the tests a bit.

I think the contents of test_binary_ops.py, test_numpy.py and test_scalar_ops.py should probably all be in one file. Then there are quite a few tests that are effectively duplicated within those three files, and tests which are only testing scalar functionality (in test_scalar_ops) which should be using unary_ufunc_check or binary_ufunc_check.

@j-towns
Copy link
Collaborator Author

j-towns commented May 10, 2017

For example, arcsin, arccos, and arctan are currently only tested on scalars but should be tested as unary_ufuncs.

@coveralls
Copy link

coveralls commented May 10, 2017

Coverage Status

Changes Unknown when pulling 8c3e4e5 on j-towns:coverage into ** on HIPS:master**.

@j-towns
Copy link
Collaborator Author

j-towns commented May 10, 2017

I've disabled those autogenerated coveralls comments.

@mattjj
Copy link
Contributor

mattjj commented May 10, 2017

Wow, awesome work here. Improving the tests and estimating coverage would be really valuable. The tests have saved me from committing bad or incomplete changes on many occasions.

Is that estimate of the total test coverage accurate? I thought we had tests for pretty much every vjp and the core functionality as well, but maybe this total is including other code.

@j-towns
Copy link
Collaborator Author

j-towns commented May 10, 2017 via email

@j-towns
Copy link
Collaborator Author

j-towns commented May 10, 2017 via email

@mattjj
Copy link
Contributor

mattjj commented Jun 4, 2017

I like the suggestions in your most recent comment (wow, has it already been 25 days?). I think it is important to have check_grads accessible from the autograd package, since it's such a handy function when writing custom VJPs or even when convincing yourself that autograd is doing the right thing on your code. However, I'm not sure how to make an alias to tests/numpy_utils.py:check_grads without also including the tests directory in the autograd package.

Maybe we can have an alias of check_grads in numpy_utils.py somehow, with the definition still in the autograd package?

@j-towns
Copy link
Collaborator Author

j-towns commented Jun 5, 2017

Yeah actually doing it that way around makes more sense given what you've pointed out about the packages.

I'll have a go at implementing that when I've got time (am pretty busy at the moment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants