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

Tidied up imports and testing #2

Merged
merged 7 commits into from
Oct 24, 2012

Conversation

astrofrog
Copy link
Contributor

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:

  • including 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 update setup.py appropriately. Note that runtests.py contains everything needed to run py.test, so no need to install it as a separate dependency.
  • changing all the unittest specific syntax to normal asserts, which py.test understands - this makes the asserts more readable in my opinion
  • removing all absolute imports in astrodendro

To run tests:

python setup.py test

Note that one test is failing due to a change from assertItemsEqual to assert np.all(x == y) which I am not sure if it is correct. @bradenmacdonald - what were you intending to do with assertItemsEqual? 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!

@ChrisBeaumont
Copy link
Contributor

Looks good to me! I have some additional refactoring + testing ideas that I promise to spend some time on in the near future

@@ -109,9 +109,9 @@ Unit Tests and Benchmarks
Several unit tests are included, and are also installed with the package.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line needs to be updated, as I believe the tests and benchmarks are no longer installed with the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are still installed with the package, and I'm adding the benchmark back too.

@bradenmacdonald
Copy link
Member

@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

@astrofrog
Copy link
Contributor Author

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.

@astrofrog
Copy link
Contributor Author

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.

@astrofrog
Copy link
Contributor Author

@bradenmacdonald - I thin I've implemented your suggestions - could you double check? Is this ok to merge?

@bradenmacdonald
Copy link
Member

👍 Looks great!

astrofrog added a commit that referenced this pull request Oct 24, 2012
@astrofrog astrofrog merged commit 14a93db into dendrograms:master Oct 24, 2012
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