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

Pytest Style coordinates/test_xdr.py #1655

Merged
merged 2 commits into from
Sep 5, 2017

Conversation

utkbansal
Copy link
Member

@utkbansal utkbansal commented Aug 26, 2017

@richardjgowers @kain88-de @jbarnoud The tests in _GromacsReader_offsets are failing.
My best guess is that it is because of the line shutil.copy(self.filename, str(tmpdir))(is in the setUp method). What have I done wrong?

@kain88-de
Copy link
Member

Can you run those tests locally?

@utkbansal
Copy link
Member Author

@kain88-de No, as I said they fail after I did my changes.

@kain88-de
Copy link
Member

Can you isolate the change responsible?

@orbeckst
Copy link
Member

@utkbansal could you please also remove importorskip('netCDF4') (if it is still there in test_xdr.py)? See #1650 (comment)

Thanks!

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

See comments on the offsets.

Please also remove the importorskip('netCDF4').

"""Issue 117: Cannot write XTC or TRR from AMBER NCDF"""
ext = None
prec = 5

def setUp(self):
@pytest.fixture()
def universe(self):
pytest.importorskip('netCDF4')
Copy link
Member

Choose a reason for hiding this comment

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

Please remove pytest.importorskip('netCDF4') - not needed anymore since #503. Thanks.

@pytest.fixture()
def ts(self, trajectory):
return trajectory.ts


def setUp(self):
Copy link
Member

Choose a reason for hiding this comment

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

Don't you have to delete setUp, now that you have the fixtures?

@pytest.fixture()
def ts(self, trajectory):
return trajectory.ts


def setUp(self):
# since offsets are automatically generated in the same directory
Copy link
Member

Choose a reason for hiding this comment

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

Put this 2-line comment before the new fixtures – it is important in order to understand why we even bother to copy trajectories around.

def trajectory(self, traj):
return self._reader(traj)

@pytest.fixture()
Copy link
Member

Choose a reason for hiding this comment

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

The ts fixture seems overkill; I don't think it is wrong but looks overly complicated. I would rewrite the tests to use trajectory.ts instead of ts.

@@ -751,6 +720,21 @@ class _GromacsReader_offsets(TestCase):
ref_volume = 362270.0
ref_offsets = None
_reader = None
prec = 3

@pytest.fixture()
Copy link
Member

Choose a reason for hiding this comment

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

I think, this must be a function/method level-fixture. It cannot be re-used for the whole class. I don't know if this is its default scope. — @kain88-de please correct me if I am wrong here.

The reason is that MDAnalysis creates a hidden file when it reads the trajectory the first time (the so-called offsets file). This hidden file is stored in the same directory as the trajectory file. The tests here are supposed to test what happens when this hidden file does not exist yet.

Copy link
Member

Choose a reason for hiding this comment

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

@orbeckst by default things are function scoped

@kain88-de
Copy link
Member

@orbeckst can you have another look. The tests should be fixed now.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

@kain88-de Thanks for fixing up everything. Sorry, I have a few more change requests, primarily just using context managers for the writers when possible. Please ping me when you want me to look at it again.


@pytest.fixture(scope='class')
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to have only one universe per class, given that various tests rewind or use next?

Or does parallel pytest create one class-scoped fixture per process so that there is no parallel collision?

I assume the latter... just asking for clarification.

Copy link
Member

Choose a reason for hiding this comment

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

There is an experimental grouping option so that every test in a class runs on the same process. This isn't guaranteed currently. But I haven't found any errors so far. I'm also not sure when fixtures are evaluated. This is because all tests are scanned by all workers before xdist runs all tests. This likely means that each process gets it's own copy and tmp directory.

# coordinates in the base unit (needed for True)
ca_Angstrom = ca_nm * 10.0
U = self.universe
U = universe
Copy link
Member

Choose a reason for hiding this comment

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

This aliasing seems superfluous, and further down you use universe instead of U. I would just use universe everywhere.

assert_equal(self.universe.atoms.n_atoms, W.n_atoms)
def test_get_Writer(self, universe, tmpdir):
ext = os.path.splitext(self.filename)[1]
outfile = str(tmpdir.join('/xdr-reader-test' + ext))
Copy link
Member

Choose a reason for hiding this comment

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

Do you need the initial / for tmpdir.join() or does it insert the separator by itself?

def test_get_Writer(self, universe, tmpdir):
ext = os.path.splitext(self.filename)[1]
outfile = str(tmpdir.join('/xdr-reader-test' + ext))
W = universe.trajectory.Writer(outfile)
Copy link
Member

Choose a reason for hiding this comment

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

Please use

with universe.trajectory.Writer(outfile) as W:
   ...

instead of the explicit W.close(). The context manager is safer and also the "best practice".

Copy link
Member

Choose a reason for hiding this comment

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

I was simply to lazy to change all the occurrences.

universe = mda.Universe(GRO, self.filename, convert_units=True)
ext = os.path.splitext(self.filename)[1]
outfile = str(tmpdir.join('/xdr-reader-test' + ext))
W = universe.trajectory.Writer(outfile)
Copy link
Member

Choose a reason for hiding this comment

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

Please use context manager.

for ts in self.universe.trajectory:
def test_velocities(self, universe, Writer, outfile):
t = universe.trajectory
W = Writer(outfile, t.n_atoms, dt=t.dt)
Copy link
Member

Choose a reason for hiding this comment

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

Context manager?

W = self.Writer(self.outfile, t.n_atoms, dt=t.dt)
for ts in self.universe.trajectory:
def test_velocities(self, universe, Writer, outfile):
t = universe.trajectory
Copy link
Member

Choose a reason for hiding this comment

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

Gratuitous aliasing ;-) – but that's a matter of taste.

W = self.Writer(self.outfile, t.n_atoms, dt=t.dt)
for ts in self.universe.trajectory:
t = universe.trajectory
W = Writer(outfile, t.n_atoms, dt=t.dt)
Copy link
Member

Choose a reason for hiding this comment

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

Context manager?

t = self.universe.trajectory
W = self.Writer(self.outfile, t.n_atoms, dt=t.dt)
for ts in self.universe.trajectory:
t = universe.trajectory
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just write it out in the next line...

self.Writer.close()
universe.trajectory
outfile = str(tmpdir.join('/xdr-writer-issue117' + self.ext))
Writer = mda.Writer(outfile, n_atoms=universe.atoms.n_atoms)
Copy link
Member

Choose a reason for hiding this comment

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

Context manager?

@orbeckst orbeckst changed the title [Review Needed]Pytest Style coordinates/test_xdr.py Pytest Style coordinates/test_xdr.py Sep 5, 2017
@kain88-de
Copy link
Member

@orbeckst ping.

@orbeckst
Copy link
Member

orbeckst commented Sep 5, 2017

Just waiting for travis.

I will squash it all together. I think this will credit the commit to @kain88-de because the first commit was already committed with him. Is this ok, @utkbansal and @kain88-de ? Alternatively, rewrite the history yourself into two separate commits. But I would want to avoid multiple minor commits.

- fix failing tests
- remove setUp method
- remove array asserts
- clean up
- restore old comment
- style fixes
- use best practices with context manager
- class scope for fixture
@kain88-de
Copy link
Member

@orbeckst please rebase or merge normally. Do not squash.

@kain88-de
Copy link
Member

btw travis builds can take a good while now. The osx builds take forever to start.

@orbeckst
Copy link
Member

orbeckst commented Sep 5, 2017

Then can you please rewrite the history and squash all your commits yourself? I will then merge the two commits, one from @utkbansal, the other from you. But I don't see a point in having all these little changes in a single file split into multiple commits – or have you got specific reasons for keeping your commits separate?

@kain88-de
Copy link
Member

@orbeckst done

@orbeckst
Copy link
Member

orbeckst commented Sep 5, 2017

Thanks!! ... just waiting for the green checkmark from Travis and will then merge.

@orbeckst
Copy link
Member

orbeckst commented Sep 5, 2017

The macOS tests have not been scheduled to run yet on Travis but everything else passes. Therefore, I am going to merge.

@orbeckst orbeckst merged commit 5e2d0ba into MDAnalysis:develop Sep 5, 2017
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

4 participants