Cython update, Unittests, and Python 3 support

[wackywendell]

I've now updated the code to support Python 3 (in addition to Python 2), and added the unittests to go with it. Naturally, this also required a Cython update. I also updated the distutils classifiers, partly to indicate this dual support.

Testing on Python 2.7 and Python 3.4:

$ nosetests2 --with-coverage pyvoro
..........
Name     Stmts   Miss  Cover   Missing
--------------------------------------
pyvoro      83      0   100%   
----------------------------------------------------------------------
Ran 10 tests in 0.007s

OK
$ nosetests --with-coverage pyvoro
..........
Name     Stmts   Miss  Cover   Missing
--------------------------------------
pyvoro      83      0   100%   
----------------------------------------------------------------------
Ran 10 tests in 0.006s

OK

The unittest module is included all the way back to Python 2.1, and from __future__ import division is as old as Python 2.2, so although I only tested Python 3.4 and Python 2.7, this will probably work with Python versions as old as that.

The "100% coverage" above just means that I used both compute_voronoi and compute_2d_voronoi functions in the unit test, but a careful look at the tests will show that I tested all five aspects of the cells: volume, original position, vertices, faces, and adjacency. This was only done on a system with 2 particles, but if it works for those, I expect it should work in general... at least as much as the Python code is involved. The unit tests may not provide complete coverage of the C++ code, but that is unaffected by this pull request.

[joe-jordan]

Thanks for your work on this, but these are quite weird unit tests.

Could you please remove anywhere where floats are compared for equality with a constant, as this may break on processors with slightly different designs, and instead compare with, say, 1% accuracy required.

Equally, it is much better practise to generate random test data and then check that the output is of the correct format, with approximately correct values, than to test with hard coded trivial examples. For example, rather than testing each cell volume, test that their sum is within 1% of the expected value (which is entirely possible with random-length random-value data.)

Checking that keys are present is enough, we don't need to verify every type of value (although making sure they're within valid ranges would be good, e.g. within the system box.) We should also, eg, test that there are the correct number of cells, but not every value in every cell.

I'm not sure if you've done this, but you should also run expected error cases, and check that the code throws the appropriate exception (I believe a VoroPlusPlusError is defined somewhere.) This is actually very important, as some inputs may cause segfaults if there is an error in what the python code passes to C++.

[wackywendell]

Could you please remove anywhere where floats are compared for equality with a constant

Those are already covered by AssertAlmostEqual, which is provided by the unittest module for exactly this reason.

it is much better practise to generate random test data

Hm, that's interesting; I'd always been taught not to use random data - maybe randomly generated once, but not randomly each time - because with random data, you might end up with a bug that appears to be a Heisenbug, not appearing on the initial commit that introduced, maybe not appearing for 100 commits, and breaking a build somewhere down the line with no particular connection to that particular build.

you should also run expected error cases, and check that the code throws the appropriate exception

Yes, that's not a bad idea. However, looking at the code, VoronoiPlusPlusError is only raised when "number of cells found was not equal to the number of particles"; I'm not exactly sure what would trigger that. Do you know? Other than that, what are appropriate exceptions for the wrong input? I can't find any documentation of that.

Also, I understand why you're asking for unit tests - they are definitely good to have - but I find your exacting standards a bit surprising considering that this project doesn't have a single one at the moment. Do you think that the small number of changes I've added are more detrimental to the project than the combination of Python 3 support and the unit tests I've added?

[wackywendell]

Also, I'd like to mention that compiling the .h, .cpp, and .pyx files by necessity results in exactly the same behavior under Python 2 as under Python 3; .pyx files are written in the "Cython" language which uses Python 2 syntax.

[joe-jordan]

Apologies, I want to make sure that there are no behaviour differences between pythons 2 and 3, which means testing quite a lot.

My exacting standards are why I've not yet got around to writing unit tests! (that, and the fact that a large portion of the codebase wasn't mine, and I wanted to test that too.) Let's forget them for now, mostly.

Can we generate 5 sets of static random data, rather than 1? I think that's the best of both worlds for now, and should be doable with a little refactoring. You raise a very good point about unfindable bugs.

To force a VoronoiPlusPlusError, just run a tessellation with a point outside the box. The original C++ just gives you less cells in this case, but because I want the cells to come out in the same order that the points went in, I disallow that result.

Thanks, I think we're nearly there :)

[joe-jordan]

Oh, re: Cython, are you sure? Might Cython not use preprocessor flags to swap out different code for python 2 and 3? It can see that data at compile time.

[wackywendell]

Hi, I just thought I'd let you know that I decided to go back to working on my own bindings, tess. Its a bit of a different interface, using Python classes instead of a dict, with some more methods from voro++ tacked on and a couple other things too. Anyways, just thought I'd let you know!