Closed astrofrog closed 11 years ago
Looks good to me! I have some additional refactoring + testing ideas that I promise to spend some time on in the near future
@astrofrog, this looks great. Personally, I prefer that tests and benchmarks get installed with the package (helps when providing support to end users since you can then ask them to run tests even if they didn't install the package themselves), but I don't think it's a big deal.
As for assertItemsEqual
, what that does is check that two arrays contain the same elements regardless of the order of the elements. It does in fact check the values, and not just the lengths of the elements, as you can see in my short proof below.
I think what you need to replace it is np.all(np.sort(x)==np.sort(y))
- see below.
import unittest
class tester(unittest.TestCase):
def test_1(self):
# These should be equal - each has a `5` and a `(1,2,3)`
self.assertItemsEqual([(1,2,3),5], [5,(1,2,3)])
def test_2(self):
# These should not be equal iff values are being checked correctly
self.assertItemsEqual([(1,2,3),5], [5,(5,5,5)])
def runTest(self):
pass
>>> t = tester()
>>> t.test_1()
# No output indicates that this test passed, i.e. the arrays are equal
>>> t.test_2()
AssertionError: Element counts were not equal:
First has 1, Second has 0: (5, 5, 5)
First has 0, Second has 1: (1, 2, 3)
>>> import numpy as np
>>> flux1 = np.array([1,2,3])
>>> flux2 = np.array([3,2,1])
>>> np.all(coords1==coords2)
False
>>> np.all(np.sort(flux1)==np.sort(flux2))
True
Ah-ha, I didn't realize it also checked the values without worrying about the order. I'll fix that. I'll also add the benchmark back to the source tree.
Note that the next thing I would like to do is make it so that tests can be simply run with
import astrodendro
astrodendro.test()
for exactly the reason @bradenmacdonald suggests, that we want to be able to provide support to users who have installed it and no longer have the source tree. But I'll leave that for the next PR.
@bradenmacdonald - I thin I've implemented your suggestions - could you double check? Is this ok to merge?
:+1: Looks great!
This PR changes from using unittest to py.test for testing (which has a number of advantages, such as parametrized tests, coverage tests, etc.). This involves:
runtests.py
at the root of the package directory (this should not be moved to e.g.scripts
) as it should not be installed, and updatesetup.py
appropriately. Note thatruntests.py
contains everything needed to run py.test, so no need to install it as a separate dependency.unittest
specific syntax to normal asserts, which py.test understands - this makes the asserts more readable in my opinionastrodendro
To run tests:
Note that one test is failing due to a change from
assertItemsEqual
toassert np.all(x == y)
which I am not sure if it is correct. @bradenmacdonald - what were you intending to do withassertItemsEqual
? That function just asserts that the elements have elements with the same length, but not that the actual values are equal - is that intentional?I also used this as a chance to fix PEP8 warnings, but it makes the diff harder to read. To ignore whitespace changes, add
?w=0
to the URL once you are viewing the diff.I also moved
benchmark.py
outside the source package, as I don't think it belongs in the installed source.@bradenmacdonald and @ChrisBeaumont - let me know what you think!