Port GRO tests to new BaseReader/Writer Test classes #1196

utkbansal · 2017-02-03T07:19:11Z

Fixes part of #516

Changes made in this Pull Request:

Modified GROWriter to conform to the Reader API standard (works with Universe, Timestep & AtomGroup).
Breaks BaseReaderTest into BaseReaderTest and MultiframeReaderTest for single frame & multi frame readers.
All tests now inherit from Base reader/writer test classes.

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

utkbansal · 2017-02-03T07:23:10Z

@jbarnoud When I ran the create_data.py script, the old test files test.xyz and test.trr got changed. Is this okay? I wasn't expecting this.

jbarnoud · 2017-02-03T08:23:50Z

I do not see why the frames were numbered from 0, and are now numbered from 1. After a quick glance at the code, I would have expected them to be numbered from 0; yet, I am not super up to date with the readers. Maybe @kain88-de has an idea?

kain88-de

The start looks good. But GRO files are an special in that they only store a single snapshot. This means our base test will have to be adopted for such a format.

kain88-de · 2017-02-03T08:06:01Z

testsuite/MDAnalysisTests/data/coordinates/test.xyz

@@ -6,28 +6,28 @@ frame 0
      CA     9.00000   10.00000   11.00000
      CA    12.00000   13.00000   14.00000
 5
-frame 0
+frame 1


This looks like we fixed something in the xyz writer since we created these files.

Please also create a new zipped file for the xyz format based on this. The readme says how this is done.

this file should be removed too

kain88-de · 2017-02-03T08:07:36Z

testsuite/MDAnalysisTests/coordinates/test_gro2.py

+import numpy as np
+
+
+class GROReference(BaseReference):


they should be included in the main gro test file.

Yes, I just wanted to keep it separate while I'm working on it so it don't get it mixed. Once I'm done I'll move it to its correct place.

kain88-de · 2017-02-03T08:17:52Z

testsuite/MDAnalysisTests/data/coordinates/test.gro

+    3ILE     CA    3   9.600  11.200  12.800  0.9600  1.1200  1.2800
+    4LYS     CA    4  14.400  16.000  17.600  1.4400  1.6000  1.7600
+    5LEU     CA    5  19.200  20.800  22.400  1.9200  2.0800  2.2400
+   8.51000   8.59223   8.35003   0.00000   0.00000   0.69131   0.00000   1.45589   2.09054


This looks like it's only the last frame. But according to the docs GRO can only write one frame per file. Multiple frames are stored in multiple files. So the trajectory in gro files can only be read with the chain-reader (that works and it tested separately no need to test the chain reader in this PR)

kain88-de · 2017-02-03T08:18:28Z

testsuite/MDAnalysisTests/data/coordinates/test.gro

@@ -0,0 +1,8 @@
+Written by MDAnalysis


This header line should include a time t = as mentioned in the gromacs docs.

This will be an issue with the GROWriter then?

t is optional according to docs

Yes but we have that information so we should provide it.

kain88-de · 2017-02-03T08:22:20Z

testsuite/MDAnalysisTests/coordinates/test_gro2.py

+        self.ext = 'gro'
+        # self.volume = 0
+        # self.dimensions = np.zeros(6)
+        # self.container_format = True


You need to deactivate all tests that skip frames or otherwise work on the assumption that a gro stores a trajectory instead of a single snapshot. For this you need to change the BaseReaderTest.

@kain88-de I looked into how tests are skipped and I came across the skipif decorator that we import from numpy. But then to check the ext attribute I'd have to use self which I can't. I mean I can't do something like:

@dec.skipif(self.ext == 'gro')

Is there a workaround for this? Or should I fall back to vanilla if/else statements in methods?

General comment. The general purpose BaseReaderTest shouldn't know anything about the specific extensions used. You should enable/disable tests based on values in the reference.

Why doesn't the skipif work?

@kain88-de It doesn't work because there isn't any reference to self.

It's probably best to make BaseReaderTest a subclass of a base class for single frame readers? The TRR readers etc do everything that GROReader does, with some extra features (traj iteration)

So the base class has what a GROReader should have. BaseReaderTest should then inherit from it and add functionality for TRR etc. that have multiple frames? Or maybe we could have a multiple frame mixin 🤔

@utkbansal yeah so something like

class BaseReaderTest(object): # define what ALL readers should do class MultiframeReaderTest(BaseReaderTest): # define what extra a multiframe reader does

So the current existing BaseReaderTest is actually a mix of two sets of tests imo

kain88-de · 2017-02-03T08:27:11Z

testsuite/MDAnalysisTests/data/coordinates/test.gro

@@ -0,0 +1,8 @@
+Written by MDAnalysis
+    5
+    1MET     CA    1   0.000   1.600   3.200  0.0000  0.1600  0.3200


The last 3 values are the velocities. You should also check that they are activated in the tests

utkbansal · 2017-02-04T08:32:57Z

@kain88-de Can you have a look at the travis build errors. I don't understand the errors. Though there are errors in the code, but the traceback is different.

kain88-de · 2017-02-04T08:41:45Z

lets see what happens when we restart the build

utkbansal · 2017-02-04T08:59:49Z

@kain88-de Same error TypeError: sequence item 0: expected string or Unicode, exceptions.TypeError found Weird thing is that no part of the traceback has any mention of my code, its all from nose

The only reference to our code is at open_files.py which I didn't touch.

jbarnoud · 2017-02-04T09:13:49Z

The traceback is because of my nose plugin that look at open files. My last "fix" on it obviously made it more fragile. I'll deactivate it from travis.

utkbansal · 2017-02-04T10:49:51Z

@jbarnoud @kain88-de Thanks! the flag --no-open-files almost fixes the bug and I get the correct tracebacks.
So back to the original question that I wanted to ask, 7 tests are currently failing which compare values with the desired values that are set in the BaseReference. How do I know the correct desired value to set in the BaseReference?

eg, MDAnalysisTests.coordinates.test_gro2.TestGROReader.test_first_dimensions fails with the following error -

Arrays are not almost equal to 6 decimals

(mismatch 100.0%)
 x: array([ 85.100006,  86.199959,  87.300041,  75.400002,  80.400002,
        85.400024], dtype=float32)
 y: array([ 81.099998,  82.199997,  83.300003,  75.      ,  80.      ,  85.      ], dtype=float32)

Here the value y was already set in the BaseReference, my guess is that I will have to change it to an appropriate value for the GRO format. How do I get the correct value?

richardjgowers · 2017-02-04T11:00:12Z

Hey, this is good work. I think the problem here is that the Gro file is the last frame (it gets overwritten repeatedly until the last frame). So the reference is correct, just the write method which creates the test Gro file should only write the first frame.

utkbansal · 2017-02-04T11:18:32Z

@richardjgowers Okay. So I will have to modify the create_data.py script to output only the first frame in case of gro, right?

richardjgowers · 2017-02-04T11:34:55Z

Yes

…

On Sat, 4 Feb 2017, 11:18 a.m. Utkarsh Bansal, ***@***.***> wrote: @richardjgowers <https://github.com/richardjgowers> Okay. So I will have to modify the create_data.py script to output only the first frame in case of gro, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1196 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jBw8N1jazX2z36AsO5ZYeeooktM3mks5rZF6IgaJpZM4L2B2S> .

utkbansal · 2017-02-04T11:51:44Z

@kain88-de @richardjgowers I have updated the gro file. Can you confirm if this is the first frame?

utkbansal · 2017-02-04T13:18:45Z

In the last commit I added velocities to the GROReference, but I have multiplied each velocity value in the gro file by 10 to compensate for the change in units.

kain88-de · 2017-02-06T21:59:06Z

@utkbansal It looks like you need to patch the GroWriter since it isn't following the standard right now. You will also have to split the Writer tests into a BaseWriter and MultiFrameWriter tests.

utkbansal · 2017-02-06T22:08:27Z

@kain88-de You mean the GROWriter should also have a n_atoms attribute?
Also what about TestGROReader.test_iter_as_aux_lowf , will it also be a part of MultiframeReader?

kain88-de · 2017-02-07T08:15:31Z

You mean the GROWriter should also have a n_atoms attribute?

yes

Also what about TestGROReader.test_iter_as_aux_lowf, will it also be a part of MultiframeReader

@jbarnoud you know most about the aux reader

jbarnoud · 2017-02-07T08:18:52Z

TestGROReader.test_iter_as_aux_lowf tests if the aux reader manage to align data and frames when the data have a lower frequency as the trajectory. As it is about iterating over trajectory frames, it is only relevant when there are multiple frames.

utkbansal · 2017-02-07T14:29:32Z

@jbarnoud @kain88-de Regarding MDAnalysisTests.coordinates.test_gro2.TestGROReader.test_total_time testcase, should the value of the total time come out to be 4 or will it be 0 because we have only one frame?

richardjgowers · 2017-02-07T15:37:21Z

@utkbansal 0

utkbansal · 2017-02-07T18:08:43Z

@kain88-de I don't see any frame skipping stuff in BaseWriterTest so I'm not sure what should be shifted to MultiFrameWriter.

kain88-de · 2017-02-12T20:56:55Z

@utkbansal the writer tests often writes multi frame trajectories. The GRO format doesn't allow to write multiple frames into one file. So the Writer tests should only check that the one frame it can write is written correctly.

utkbansal · 2017-02-14T20:30:31Z

@kain88-de @richardjgowers I have added all the tests required for the GROReader class, a couple of them are failing right now and I'll need help with them.

Regarding tests for GROWriter, I think we need to add some parts to the GROWriter class to make its working consistent with what the XYZWriter does. Specifically this part in the XYZWriter has no equivalent in the GROWriter class, due to which tests in TestGROWriter are failing.

utkbansal · 2017-02-22T20:53:13Z

@kain88-de @richardjgowers @jbarnoud I need help with this please.

kain88-de · 2017-02-23T21:32:32Z

package/MDAnalysis/coordinates/base.py

@@ -1747,7 +1747,7 @@ class SingleFrameReader(ProtoReader):
    """
    _err = "{0} only contains a single frame"

-    def __init__(self, filename, convert_units=None, **kwargs):
+    def __init__(self, filename, convert_units=None, n_atoms=None, **kwargs):


why did you make this change?

kain88-de · 2017-02-23T21:34:17Z

testsuite/MDAnalysisTests/data/coordinates/test.xyz

@@ -6,28 +6,28 @@ frame 0
      CA     9.00000   10.00000   11.00000
      CA    12.00000   13.00000   14.00000
 5
-frame 0
+frame 1


Please also create a new zipped file for the xyz format based on this. The readme says how this is done.

kain88-de · 2017-02-23T21:38:04Z

Regarding tests for GROWriter, I think we need to add some parts to the GROWriter class to make its working consistent with what the XYZWriter does. Specifically this part in the XYZWriter has no equivalent in the GROWriter class, due to which tests in TestGROWriter are failing.

The GROWriter currently doesn't follow the standard API that we have. So you will need to change that one to accept and work with a Timestep, AtomGroup, or a Universe. The frame argument should be removed. Please also rename selection to obj and document that it can be any of the types specified above.

Writing a Universe or AtomGroup you should be able to use the current code. For the Timestep you have to make sure that valid names and resnames exist. The standard for unknown atom name is X and for the resname UNK. We currently already do this for AtomGroups that don't provide a names or resnames.

* Use .copy() method to use new copies instead of references

utkbansal · 2017-03-23T10:13:49Z

@kain88-de @richardjgowers I have done a rebase and force push.

richardjgowers · 2017-03-23T10:15:29Z

LGTM assuming tests pass

kain88-de · 2017-03-23T11:51:19Z

@utkbansal thanks for doing this all the way to the end

utkbansal · 2017-03-23T16:38:42Z

I should be the one thanking you all 😄 Without help from all of you I wouldn't be able to do this.

kain88-de requested changes Feb 3, 2017

View reviewed changes

kain88-de reviewed Feb 3, 2017

View reviewed changes

utkbansal force-pushed the issue-516-GRO branch from b4d818b to f882a29 Compare February 4, 2017 05:52

jbarnoud mentioned this pull request Feb 4, 2017

Deactivate open-files plugin in travis #1198

Merged

utkbansal force-pushed the issue-516-GRO branch from 3ad9ff2 to 71ce217 Compare February 7, 2017 12:27

utkbansal force-pushed the issue-516-GRO branch from 77c6aea to 26e1b4c Compare February 7, 2017 16:50

kain88-de previously requested changes Feb 23, 2017

View reviewed changes

utkbansal added 22 commits March 23, 2017 14:56

Adds n_atoms attribute to GROWriter

b9370fc

Set totaltime in GROReference

26094e7

Adds basic TestGROWriter

3ed7edb

Adds basic TestGROReaderNoConversion class

ce7a063

Adds postion, velocity and other attrs to GRONoConversionReference

8d1442a

Adds TestGROReaderIncompleteVelocities class

989c9e5

Remove n_atoms and rename selection to obj in GROWriter

e693c28

Adds TestGROBZ2Reader and bz2 compressed GRO datafile

92b80b7

Modify GROWrite's write method to handle Timestep

27687c2

Fixes in write method so that timestep is not modified

d13ea3e

* Use .copy() method to use new copies instead of references

Adds TestGROWriterIncompleteVelocities and TestGROBZ2Writer class

07adb5a

Adds aux data to reader in TestGROReaderNoConversion

fd505a8

Modifies GRO Writer to not convert AtomGroup to Timestep

8113009

Postion conversion is now not inplace

f4357fc

Minor refactor in GROWriter

3a0039f

Minor doc fixes and adds back **kwargs argument

343d29e

Rename selection to ag_or_ts & update docstring

404cc86

Adds tests to TestGROWriter, TestGROLargerWriter, TestGROTimestep

c75e554

Adds tests to TestGROReader

5437660

Merged test_gro and test_gro2

0625636

Revert changes to test.trr

c782eea

Revert changes to text.xyz

1a3f81f

utkbansal force-pushed the issue-516-GRO branch from 38f631f to 1a3f81f Compare March 23, 2017 09:29

richardjgowers added Format-Gromacs testing labels Mar 23, 2017

richardjgowers merged commit 2ff2ee6 into MDAnalysis:develop Mar 23, 2017

Port GRO tests to new BaseReader/Writer Test classes #1196

Port GRO tests to new BaseReader/Writer Test classes #1196

Conversation

utkbansal commented Feb 3, 2017 • edited Loading

PR Checklist

utkbansal commented Feb 3, 2017

jbarnoud commented Feb 3, 2017

kain88-de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utkbansal Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utkbansal Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utkbansal commented Feb 4, 2017

kain88-de commented Feb 4, 2017

utkbansal commented Feb 4, 2017 • edited Loading

jbarnoud commented Feb 4, 2017

utkbansal commented Feb 4, 2017 • edited Loading

richardjgowers commented Feb 4, 2017

utkbansal commented Feb 4, 2017

richardjgowers commented Feb 4, 2017 via email

utkbansal commented Feb 4, 2017 • edited Loading

utkbansal commented Feb 4, 2017

kain88-de commented Feb 6, 2017

utkbansal commented Feb 6, 2017 • edited Loading

kain88-de commented Feb 7, 2017

jbarnoud commented Feb 7, 2017

utkbansal commented Feb 7, 2017

richardjgowers commented Feb 7, 2017

utkbansal commented Feb 7, 2017

kain88-de commented Feb 12, 2017

utkbansal commented Feb 14, 2017

utkbansal commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de commented Feb 23, 2017

utkbansal commented Mar 23, 2017

richardjgowers commented Mar 23, 2017

kain88-de commented Mar 23, 2017

utkbansal commented Mar 23, 2017

utkbansal commented Feb 3, 2017 •

edited

Loading

utkbansal Feb 3, 2017 •

edited

Loading

utkbansal Feb 3, 2017 •

edited

Loading

utkbansal commented Feb 4, 2017 •

edited

Loading

utkbansal commented Feb 4, 2017 •

edited

Loading

utkbansal commented Feb 4, 2017 •

edited

Loading

utkbansal commented Feb 6, 2017 •

edited

Loading