Convert libmdaxdr test by kain88-de · Pull Request #1442 · MDAnalysis/mdanalysis

kain88-de · 2017-06-27T21:41:16Z

Changes made in this Pull Request:

libmdaxdr is ported to pytest fully
new tests based on libdcd
changed behavior of libmdaxdr to accept a broader range of inputs

PR Checklist

Tests?
Docs?
CHANGELOG updated?
~~- [x] Issue raised/referenced?~~

richardjgowers · 2017-06-28T06:14:13Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+    assert frame.lmbda == 0.5


-class XDRFormatBaseTest(object):


You could have kept the class and parametrised it - do you prefer the standalone functions instead?

I prefer the stand alone funtions. A neat thing about them is that I see all variables effecting a test at once. No need to scroll and it is immediately clear which files are going to be tested.

richardjgowers · 2017-06-28T06:16:24Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

-        assert_equal(f_out.n_atoms, 0)
-        for frame in f_in:
+@pytest.mark.parametrize('dtype', (np.int32, np.int64, np.float32, np.float64, int, float))
+def test_write_trr_dtype(tmpdir, dtype):


Something I have in another project is a in_tmpdir fixture, which is something like:

def in_tmpdir(tmpdir): with tmpdir.as_cwd(): yield

But it then saves you another level of indentation as the fixture is dealing with teardown. Worth putting in?

then we have def test_a(in_tmpdir): which runs even more magically in a temporary directory and we don't use the in_tmpdir variable. Not using a variable isn't so nice since I would like to enable pylint to warn about unused variables soon.

So good idea. But maybe to much magic

richardjgowers · 2017-06-28T12:31:44Z

So does this conflict with anything that @utkbansal is currently working on?

kain88-de · 2017-06-28T12:35:54Z

It shouldn't he already ported formats to pytest a while ago. I just wanted to add new useful tests that @tylerjereddy wrote for libdcd.

kain88-de · 2017-06-28T12:36:24Z

I admit I might have been a bit over enthusiastic implementing pytest idoms in the tests.

richardjgowers · 2017-06-28T12:40:06Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+
+
+def test_xtc():
+    f = XTCFile(XTC_single_frame)


won't this open a file handle that isn't otherwise cleaned up?

I fixed that. I didn't see any before because the function should clean up nicely and call dealloc. But lets be on the safe side

richardjgowers · 2017-06-28T12:42:32Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+
+@pytest.mark.parametrize('dtype', (np.int32, np.int64, np.float32, np.float64, int, float))
+def test_write_xtc_dtype(tmpdir, dtype):
+    with tmpdir.as_cwd():


So maybe having a unused fixture isn't any better, but I prefer the decorator approach to tmpdir. We're only using the inside of the context manager, so it's cleaner to not have the indentation.

richardjgowers · 2017-06-28T12:42:58Z

Yeah, I think PRs like this will probably set up which idioms etc we'll use globally.

kain88-de · 2017-06-28T20:17:49Z

Yeah, I think PRs like this will probably set up which idioms etc we'll use globally.

I opened an issue #1444 to talk about this a bit.

richardjgowers · 2017-06-29T09:32:41Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+        # xtc only saves with 3 decimal places precision
+        assert_array_almost_equal(xyz.flat, np.arange(30), decimal=3)
+        assert_array_almost_equal(box, np.eye(3) * 20, decimal=3)
+        assert step == 0


I don't like having multiple asserts in one test function, it makes it harder to debug. Eg, if xyz.flat stopped working, I'd also want to know if box was working too, to help me try and locate the problem. This test wouldn't give me the breakdown of what is broken.

I know you're not a fan of classes, but ..

class TestXTC(object): @staticmethod @pytest.fixture() def XTC(): with XTCFile(XTC_single_frame) as f: yield f def test_n_atoms(self, XTC): assert XTC.n_atoms == 10 def test_xyc(self, XTC): etc

This is a lot nicer imo

I like the XTC fixture. I think I will add that one.

orbeckst

You do like pytest...

orbeckst · 2017-06-30T03:13:33Z

package/CHANGELOG


 Enhancements
+  * libmdaxdr classes now accept more argument types for a write (PR #1442)
+  * libmdaxdr classes now raise EOFError when accessing another frame after


I think this counts as a "Change", not an enhancement. State what it used to do, IOError,

orbeckst · 2017-06-30T03:16:06Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+
 from numpy.testing import assert_almost_equal
+from numpy.testing import assert_array_almost_equal
+from numpy.testing import assert_array_equal


Just out of curiosity: why not

from numpy.testing import assert_array_almost_equal, assert_array_equal

?

orbeckst · 2017-06-30T03:24:04Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

        box_compare = np.eye(3) * 20
        for i, frame in enumerate(f):
            xyz, box, step, time, prec = frame
            assert_array_almost_equal(xyz, ones * i, decimal=3)


Split into two tests? See @richardjgowers 's comment above.

kain88-de · 2017-06-30T07:14:08Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+
+@pytest.mark.parametrize("xdrfile, fname", ((XTCFile, XTC_multi_frame),
+                                            (TRRFile, TRR_multi_frame)))
+def test_over_seek(xdrfile, fname):


I'm unable in this test to use the xtc and trr fixtures above with parametrize.

kain88-de · 2017-06-30T07:19:15Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+@pytest.mark.parametrize(
+    'dtype', (np.int32, np.int64, np.float32, np.float64, int, float))
+def test_write_xtc_dtype(tmpdir, dtype, xtc):
+    outname = str(tmpdir.join('foo.xtc'))


@richardjgowers This is another option to use tmpdir. It avoids the extra indentation.

kain88-de · 2017-06-30T07:38:37Z

@utkbansal input from you would be appreciated here and in #1444 as well since you will also spend a lot of time implementing the guides we are setting now.

kain88-de · 2017-06-30T07:44:44Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+        assert f._bytes_tell() == big_offset
+
+
+def test_read_multi_frame_xtc(xtc):


I know I still need to untangle these tests

kain88-de · 2017-06-30T07:45:36Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+
+
+@pytest.mark.parametrize('array_like', (np.array, list))
+def test_write_xtc_array_like(tmpdir, array_like, xtc):


I know these tests should be untangled as well. I also think I know how to do that best giving the granularity in the tests that @richardjgowers likes.

kain88-de · 2017-07-01T21:20:25Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+
+@pytest.mark.parametrize('fname, xdr', ((XTC_multi_frame, XTCFile),
+                                        (TRR_multi_frame, TRRFile)))
+class TestCommonAPI:


@richardjgowers is that what you have in common with using classes to parametrize common tests?

I do admit that this removes a lot of boiler plate for these tests

kain88-de · 2017-07-01T21:22:45Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+
+    def test_seek_overflow(self, reader, offsets):
+        with pytest.raises(OverflowError):
+            reader._bytes_seek(2**65)


there is a bad side effect though. We need to always use all parametrized values to a function even if they are not used.

kain88-de · 2017-07-01T21:23:37Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+def test_box_xtc(xtc):
+    box = np.eye(3) * 20
+    for frame in xtc:
+        assert_array_almost_equal(frame.box, box, decimal=3)


So here I decided to go with special functions since the API of XTC and TRR are different.

kain88-de · 2017-07-01T21:25:06Z

testsuite/MDAnalysisTests/formats/test_libmdaxdr.py

+        for frame in xtc:
+            f.write(*frame)
+    with XTCFile(fname) as f:
+        yield f


these fixtures with yield are really nice. They do remove a hell of a lot of boilerplate code

richardjgowers · 2017-07-02T16:10:37Z

@kain88-de I was playing around with pytest...

@pytest.fixture(params=(
    (XTC_multi_frame, XTCFile),
    (TRR_multi_frame, TRRFile),
))
def reader(request):
    fname, cls = request.param
    with cls(fname) as f:
        yield f


class TestCommonAPI(object):
    def test_natoms(self, reader):
        assert reader.n_atoms == 10

    def test_over_seek(self, reader):
        with pytest.raises(EOFError):
            reader.seek(100)

So you can define a fixture with n params, and then all tests that use this fixture get called n times with each of the versions of the fixture.

kain88-de · 2017-07-02T16:38:48Z

@richardjgowers that is pretty cool. But then we really don't need the class anymore.

kain88-de · 2017-07-02T16:41:44Z

This pattern would yield parametrize over fixtures and single functions instead of parametrize over classes.

richardjgowers · 2017-07-02T17:20:21Z

testsuite/MDAnalysisTests/datafiles.py

 XTC_sub_sol = resource_filename(__name__, 'data/cobrotoxin.xtc')
 PDB_sub_sol = resource_filename(__name__, 'data/cobrotoxin.pdb')
 PDB_xlserial = resource_filename(__name__, 'data/xl_serial.pdb')
-XTC_single_frame = resource_filename(


Did we remove the XTC and TRR single frame files?

yes I noticed they weren't needed in the first place. They test nothing that isn't tested in the other files

kain88-de · 2017-07-07T05:47:35Z

@richardjgowers anything left to be done on this?

also improves the docs

kain88-de force-pushed the convert-libmdaxdr-test branch 2 times, most recently from 8b95a78 to 4a81a97 Compare June 28, 2017 05:58

richardjgowers reviewed Jun 28, 2017

View reviewed changes

kain88-de force-pushed the convert-libmdaxdr-test branch from 4a81a97 to 13e50d3 Compare June 28, 2017 07:49

richardjgowers reviewed Jun 28, 2017

View reviewed changes

kain88-de force-pushed the convert-libmdaxdr-test branch from 13e50d3 to b10e5e5 Compare June 28, 2017 20:16

richardjgowers reviewed Jun 29, 2017

View reviewed changes

orbeckst reviewed Jun 30, 2017

View reviewed changes

kain88-de commented Jun 30, 2017

View reviewed changes

kain88-de force-pushed the convert-libmdaxdr-test branch from caa971f to db5172a Compare June 30, 2017 07:40

kain88-de commented Jun 30, 2017

View reviewed changes

richardjgowers self-assigned this Jun 30, 2017

orbeckst added the testing label Jul 1, 2017

kain88-de commented Jul 1, 2017

View reviewed changes

kain88-de force-pushed the convert-libmdaxdr-test branch from dcee674 to ee5f872 Compare July 1, 2017 21:27

kain88-de mentioned this pull request Jul 1, 2017

pytest style guide #1444

Closed

richardjgowers reviewed Jul 2, 2017

View reviewed changes

richardjgowers approved these changes Jul 11, 2017

View reviewed changes

kain88-de added 3 commits July 11, 2017 11:33

Use EOFError in libmdaxdr when end of file is reached

247b0c8

also improves the docs

convert libmdaxdr test to proper pytest style

ad9202d

update CHANGELOG

909097b

kain88-de force-pushed the convert-libmdaxdr-test branch from ee5f872 to 909097b Compare July 11, 2017 09:34

richardjgowers merged commit 94faa8c into develop Jul 11, 2017

richardjgowers deleted the convert-libmdaxdr-test branch July 11, 2017 12:55

		assert f._bytes_tell() == big_offset


		def test_read_multi_frame_xtc(xtc):



		@pytest.mark.parametrize('array_like', (np.array, list))
		def test_write_xtc_array_like(tmpdir, array_like, xtc):

Conversation

kain88-de commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kain88-de Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardjgowers commented Jun 28, 2017

Uh oh!

kain88-de commented Jun 28, 2017

Uh oh!

kain88-de commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardjgowers commented Jun 28, 2017

Uh oh!

kain88-de commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kain88-de commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardjgowers commented Jul 2, 2017

Uh oh!

kain88-de commented Jul 2, 2017

Uh oh!

kain88-de commented Jul 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kain88-de commented Jul 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

kain88-de commented Jun 27, 2017 •

edited

Loading

kain88-de Jun 28, 2017 •

edited

Loading