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 analysis/test_hbonds.py #1544

Merged
merged 3 commits into from Aug 3, 2017

Conversation

Projects
None yet
3 participants
@utkbansal
Member

utkbansal commented Jul 23, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
'angle': 150.0,
}
def _run(self, universe, **kwargs):

This comment has been minimized.

@richardjgowers

richardjgowers Jul 24, 2017

Member

Shouldn't a method like this be replaced with a fixture? This would then make it easier to cache this result (if possible) by increasing the scope

This comment has been minimized.

@kain88-de

kain88-de Jul 25, 2017

Member

The kwargs are a problem to use a fixture here

This comment has been minimized.

@richardjgowers

richardjgowers Jul 25, 2017

Member

From what I can see it's only ever called as self._run(universe) so we could remove the kwargs?

@richardjgowers richardjgowers self-assigned this Jul 25, 2017

@richardjgowers

Should replace _run with a fixture, and look to see if we can increase the scope of it

'std': np.std,
}
def _normalize_timeseries(self, h):

This comment has been minimized.

@richardjgowers

richardjgowers Jul 26, 2017

Member

Should just put this method into the fixture rather than refer to it

@utkbansal utkbansal changed the title from [WIP]Pytest Style analysis/test_hbonds.py to Pytest Style analysis/test_hbonds.py Jul 29, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 3, 2017

@kain88-de @richardjgowers What needs to be done here?

@richardjgowers richardjgowers merged commit b6a7cbc into MDAnalysis:develop Aug 3, 2017

3 checks passed

QuantifiedCode 10 minor issues introduced.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 90.517%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment