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

Fix coverage drop in coordinates module #1441

Merged
merged 19 commits into from Jul 12, 2017
Merged

Conversation

@utkbansal
Copy link
Member

@utkbansal utkbansal commented Jun 27, 2017

Fixes #1433

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
@utkbansal utkbansal changed the title Fix coverage drop in coordinates module [WIP]Fix coverage drop in coordinates module Jun 27, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 27, 2017

With the last commit, I might have bloated the tests. I'll revisit that aspect later. I'm currently only focused on getting coverage back.

@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 28, 2017

@utkbansal how do you mean bloated? Some unnecessary tests or just linecount?

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 28, 2017

@richardjgowers In the terms of the number of lines. I should be able to make some fixtures global. Stuff like tempdir can be easily done. But I'll need to play around with scopes and see figure out the greatest scope I can use them in. But I think this work is better suited for Phase 2. Thoughst??

@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jun 28, 2017

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 28, 2017

Woah! We actually crossed 90% in coverage!

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 28, 2017

@kain88-de @richardjgowers A review at this point would be helpful. I'm mostly done porting the stuff. I now need to look for the tests that are missed out.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 28, 2017

I have restarted the build for the merge of 1414 - https://travis-ci.org/MDAnalysis/mdanalysis/builds/246515054

Want to see the drop in coverage caused by it. This PR + 1414 should ideally sum up to no drop in coverage.

with self.ref.writer(outfile, self.reader.n_atoms) as W:
for ts in self.reader:
def tmp_file(self, name, ref, tmpdir):
return tmpdir.name + name + '.' + ref.ext

This comment has been minimized.

@kain88-de

kain88-de Jun 28, 2017
Member

this might need to be changed since tmpdir is a pytest internal fixture. @utkbansal do you know where tmpdirs are created here?

def reader(ref):
reader = ref.reader(ref.trajectory)
reader.add_auxiliary('lowf', ref.aux_lowf, dt=ref.aux_lowf_dt, initial_time=0, time_selector=None)
reader.add_auxiliary('highf', ref.aux_highf, dt=ref.aux_highf_dt, initial_time=0, time_selector=None)

This comment has been minimized.

@kain88-de

kain88-de Jun 28, 2017
Member

Can we find a solution that doesn't require us to repeat this for every child class of BaseReaderTests?

This comment has been minimized.

@kain88-de

kain88-de Jul 2, 2017
Member

Can't this be defined in the base class?

def tmp_file(self, name, ref, tmpdir):
return tmpdir.name + name + '.' + ref.ext

def test_write_trajectory_timestep(self,ref, reader, tmpdir):

This comment has been minimized.

@kain88-de

kain88-de Jun 28, 2017
Member

I'm confused here. Is tmpdir here using the tmpdir-fixture of pytest? Because later you use tmpdir.name but the path.local variable that the tmpdir-fixture returns doesn't have a name attribute.

@staticmethod
@pytest.fixture()
def tmpdir():
return tempdir.TempDir()

This comment has been minimized.

@kain88-de

kain88-de Jun 28, 2017
Member

Oh man overwriting the pytest tmpdir fixture isn't a good idea. rather call it tempdir to make it distinctively different. Also this fixture shouldn't be repeated for every class that inherits BaseTestWriter. Can't it be part of the base class?

This comment has been minimized.

@kain88-de

kain88-de Jul 2, 2017
Member

I don't like that this is defined in the child class. This is common for all tests and should be defined in the base class. Similar to the universe fixtures above

This comment has been minimized.

@kain88-de

kain88-de Jul 2, 2017
Member

Also the name is NOT OK. It overwrites a pytest fixture which can lead to confusion. See above comment for alternative name suggestions


@staticmethod
@pytest.fixture()
def u_no_resnames():

This comment has been minimized.

@richardjgowers

richardjgowers Jun 29, 2017
Member

These Universe fixtures don't need to be defined in each format's tests, they're format agnostic. I would either, define them in the base class (BaseWriterTest?) or just call them in the test function. They're very light, so we're unlikely to have them has a large-scoped fixture.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 29, 2017

@richardjgowers @kain88-de Is there an easy way to compare two(or more coverage reports). Coveralls can do that but it doesn't work. I might have an idea why it doesn't work though, it would be great if we could fix it.

@kain88-de
Copy link
Member

@kain88-de kain88-de commented Jun 30, 2017

@utkbansal any progress on this?

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jun 30, 2017

@kain88-de kain88-de mentioned this pull request Jul 1, 2017
4 of 4 tasks complete
@orbeckst orbeckst added the testing label Jul 1, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 2, 2017

Missing test cases due to yield

  • test_dcd.py - Leaving this out because @kain88-de is already working on it.
  • test_reader_api.py
  • test_timestep_api.py

Other than tests in these files, all are executed.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 2, 2017

@richardjgowers @kain88-de In test_timestep_api.py we have the following test class -

class TestBaseTimestep(BaseTimestepTest):
    @dec.skipif(parser_not_found('DCD'),
                'DCD parser not available. Are you using python 3?')
    def test_other_timestep(self):
        # use a subclass to base.Timestep to check it works
        ts1 = mda.coordinates.base.Timestep(10)
        ts1.positions = self._get_pos()

        # can't do TRR Timestep here as it always has vels and forces
        # so isn't actually equal to a position only timestep
        for otherTS in [mda.coordinates.DCD.Timestep,
                        mda.coordinates.TRJ.Timestep,
                        mda.coordinates.DMS.Timestep,
                        mda.coordinates.GRO.Timestep,
                        mda.coordinates.TRZ.Timestep,
                        ]:
            ts2 = otherTS(10)
            ts2.positions = self._get_pos()
            yield (self._check_ts_equal, ts1, ts2,
                   "Failed on {0}".format(otherTS))

If we change it to something like the following, the test will be executed.

class TestBaseTimestep(BaseTimestepTest):
    @dec.skipif(parser_not_found('DCD'),
                'DCD parser not available. Are you using python 3?')
    def test_other_timestep(self):
        # use a subclass to base.Timestep to check it works
        ts1 = mda.coordinates.base.Timestep(10)
        ts1.positions = self._get_pos()

        # can't do TRR Timestep here as it always has vels and forces
        # so isn't actually equal to a position only timestep
        for otherTS in [mda.coordinates.DCD.Timestep,
                        mda.coordinates.TRJ.Timestep,
                        mda.coordinates.DMS.Timestep,
                        mda.coordinates.GRO.Timestep,
                        mda.coordinates.TRZ.Timestep,
                        ]:
            ts2 = otherTS(10)
            ts2.positions = self._get_pos()


            ##### THIS IS THE CHANGE #######
            self._check_ts_equal( ts1, ts2,
                   "Failed on {0}".format(otherTS))

This is a band-aid fix for now. But I want to do this for now because the right way will casue a lot of changes - BaseTimestepTest inherits from TestCase, I need to get rid of TestCase and use fixtures. But then I will also have to modify all the other subclasses to use fixtures too.

def reader(ref):
reader = ref.reader(ref.trajectory)
reader.add_auxiliary('lowf', ref.aux_lowf, dt=ref.aux_lowf_dt, initial_time=0, time_selector=None)
reader.add_auxiliary('highf', ref.aux_highf, dt=ref.aux_highf_dt, initial_time=0, time_selector=None)

This comment has been minimized.

@kain88-de

kain88-de Jul 2, 2017
Member

Can't this be defined in the base class?

@staticmethod
@pytest.fixture()
def tmpdir():
return tempdir.TempDir()

This comment has been minimized.

@kain88-de

kain88-de Jul 2, 2017
Member

I don't like that this is defined in the child class. This is common for all tests and should be defined in the base class. Similar to the universe fixtures above

@staticmethod
@pytest.fixture()
def tmpdir():
return tempdir.TempDir()

This comment has been minimized.

@kain88-de

kain88-de Jul 2, 2017
Member

Also the name is NOT OK. It overwrites a pytest fixture which can lead to confusion. See above comment for alternative name suggestions

@richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented Jul 2, 2017

@utkbansal with the band-aid above, why does that yield not work? Also, why can't we quickly put in the parametrize over classes?

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 2, 2017

@richardjgowers yield doesn't work if you have setUp in the class (see #1443 (comment)) and parameterize doesn't work if we inherit from TestCase (see #1443 (comment)).

To fix this properly I will have to get rid of TestCase inheritance (in BaseTimestepTest ) and then I wil have to update all the sublasses of BaseTimestepTest to use fixtures.
This can be, IMHO, done in the next phase.

@kain88-de
Copy link
Member

@kain88-de kain88-de commented Jul 2, 2017

@utkbansal this is one of the last classes and the coordinates tests are already set to pytest. You might as well go full on at least for the BaseTimestepTest. I don't mind if this takes a bit longer. You can also start working on porting the analysis tests.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 2, 2017

@kain88-de Okay, I'll fix it completely then. I'm also starting analysis - it's the only module left.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 2, 2017

@richardjgowers @kain88-de I overestimated the amount of work needed to fix this. Apologies.

@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 2, 2017

@richardjgowers @kain88-de Should be better now.

@utkbansal utkbansal changed the title [WIP]Fix coverage drop in coordinates module Fix coverage drop in coordinates module Jul 2, 2017

def test_getitem(self):
assert_equal(self.ts[1], self.refpos[1])
# def setUp(self):

This comment has been minimized.

@richardjgowers

richardjgowers Jul 3, 2017
Member

delete this


assert_raises(TypeError, sl)

def _check_getitem(self, sl):
res = [ts.frame for ts in self.reader[sl]]
@pytest.mark.parametrize('sl', [

This comment has been minimized.

@richardjgowers

richardjgowers Jul 3, 2017
Member

why can't this use a double parametrize?

This comment has been minimized.

@utkbansal

utkbansal Jul 3, 2017
Author Member

We should be able to do that.

This comment has been minimized.

@utkbansal

utkbansal Jul 3, 2017
Author Member

I'm not really sure how I can do this.

This comment has been minimized.

@richardjgowers

richardjgowers Jul 3, 2017
Member

@pytest.mark.parametrize('slice_cls', [list, np.array])
@pytest.mark.parametrize('sl', [])
def test_thing(self, slice_cls, sl):
    sl = slice_cls(sl)
    etc

This comment has been minimized.

@utkbansal

utkbansal Jul 3, 2017
Author Member

We want to turn res = [ts.frame for ts in reader[sl]] into a fixture, right? I am unable to access the reader fixture in the parametrize call.

This comment has been minimized.

@utkbansal

utkbansal Jul 3, 2017
Author Member

I think this has to do with static checking, If I define the fixture in the subclass itself, then I'm able to access it in the parameterize call.

@kain88-de
Copy link
Member

@kain88-de kain88-de commented Jul 8, 2017

@utkbansal you haven't updated the DCD tests. As I said DCD can now be updated as well since we merged the new DCD reader

utkbansal added 17 commits Jun 27, 2017
@utkbansal utkbansal force-pushed the utkbansal:coord-cov branch from 120c20b to df1b491 Jul 10, 2017
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 10, 2017

I think something is wrong with the precision in the DCD test. Can someone check?

@kain88-de
Copy link
Member

@kain88-de kain88-de commented Jul 10, 2017

@utkbansal did you have a look at the difference in these tests between your branch and develop?

@orbeckst
Copy link
Member

@orbeckst orbeckst commented Jul 10, 2017

I don't think that these failing tests should be using assert_equal when comparing floating point numbers. This should have been assert_almost_equal.

@@ -801,13 +801,13 @@ def test_time_with_offset(self):

assert_equal(ts.time, reftime + ref_offset)

def test_dt(self):
def test_dt(self, ref):

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017
Member

looks like it doesn't know what ref is. Also you forgot a test_dt in the other test class. Did you run those tests on your local laptop?

This comment has been minimized.

@utkbansal

utkbansal Jul 11, 2017
Author Member

Oddly enough the tests still fail. But if I directly change the files in where MD is installed (site-packages) then the same change works. Is there another build step I'm missing?!

This comment has been minimized.

@utkbansal

utkbansal Jul 11, 2017
Author Member

Fixed it.

Try
@utkbansal
Copy link
Member Author

@utkbansal utkbansal commented Jul 12, 2017

@kain88-de DCD works now.

@kain88-de kain88-de dismissed richardjgowers’s stale review Jul 12, 2017

Comments have been addressed

@kain88-de kain88-de merged commit 97526a5 into MDAnalysis:develop Jul 12, 2017
3 checks passed
3 checks passed
QuantifiedCode 79 minor issues introduced.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 90.337%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants