-
Notifications
You must be signed in to change notification settings - Fork 11
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
developing back end run methods #216
Changes from all commits
e71456d
6c67ec1
7bcdf49
a87cb11
7edde0d
48016c5
59c819c
fb58f27
1f7f2d1
179603f
46d3cf2
f47c806
5e02322
2fc9135
37cd1d7
0dc7e32
a0de2d9
273c1af
8212d11
653210e
7eff11f
67261cb
2f63c3a
ce02b3b
cf4bd40
f45fc6b
1a90ed8
7054bb5
19f7c84
40381aa
8870e39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -464,19 +464,35 @@ def _setup_frames(self, trajectory): | |
self.times = np.zeros(self.n_frames) | ||
|
||
def _single_universe(self): | ||
"""Calculations on a single Universe object. | ||
|
||
Run on each universe in the ensemble during when | ||
self.run in called. | ||
"""Calculations on a single :class:`MDAnalysis.Universe | ||
<MDAnalysis.core.groups.universe.Universe>` object. | ||
|
||
Run on each :class:`MDAnalysis.Universe | ||
<MDAnalysis.core.groups.universe.Universe>` | ||
in the :class:`~mdpow.analysis.ensemble.Ensemble` | ||
during when :meth:`run` in called. | ||
:exc:`NotImplementedError` will detect whether | ||
:meth:`~EnsembleAnalysis._single_universe` | ||
or :meth:`~EnsembleAnalysis._single_frame` | ||
should be implemented, based on which is defined | ||
in the :class:`~mdpow.analysis.ensemble.EnsembleAnalysis`. | ||
""" | ||
pass # pragma: no cover | ||
raise NotImplementedError | ||
|
||
def _single_frame(self): | ||
"""Calculate data from a single frame of trajectory | ||
|
||
Called on each frame for universes in the Ensemble. | ||
"""Calculate data from a single frame of trajectory. | ||
|
||
Called on each frame for each | ||
:class:`MDAnalysis.Universe <MDAnalysis.core.groups.universe.Universe>` | ||
in the :class:`~mdpow.analysis.ensemble.Ensemble`. | ||
|
||
:exc:`NotImplementedError` will detect whether | ||
:meth:`~EnsembleAnalysis._single_universe` | ||
or :meth:`~EnsembleAnalysis._single_frame` | ||
should be implemented, based on which is defined | ||
in the :class:`~mdpow.analysis.ensemble.EnsembleAnalysis`. | ||
""" | ||
pass # pragma: no cover | ||
raise NotImplementedError | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your tests do not check that the base class raises NotImplementedError. |
||
|
||
def _prepare_ensemble(self): | ||
"""For establishing data structures used in running | ||
|
@@ -505,27 +521,31 @@ def _conclude_ensemble(self): | |
pass # pragma: no cover | ||
|
||
def run(self, start=None, stop=None, step=None): | ||
"""Runs _single_universe on each system and _single_frame | ||
"""Runs :meth:`~EnsembleAnalysis._single_universe` | ||
on each system or :meth:`~EnsembleAnalysis._single_frame` | ||
on each frame in the system. | ||
|
||
First iterates through keys of ensemble, then runs _setup_system | ||
which defines the system and trajectory. Then iterates over | ||
trajectory frames. | ||
First iterates through keys of ensemble, then runs | ||
:meth:`~EnsembleAnalysis._setup_system`which defines | ||
the system and trajectory. Then iterates over each | ||
system universe or trajectory frames of each universe | ||
as defined by :meth:`~EnsembleAnalysis._single_universe` | ||
or :meth:`~EnsembleAnalysis._single_frame`. | ||
""" | ||
logger.info("Setting up systems") | ||
self._prepare_ensemble() | ||
for self._key in ProgressBar(self._ensemble.keys(), verbose=True): | ||
self._setup_system(self._key, start=start, stop=stop, step=step) | ||
self._prepare_universe() | ||
self._single_universe() | ||
for i, ts in enumerate(ProgressBar(self._trajectory[self.start:self.stop:self.step], verbose=True, | ||
try: | ||
self._single_universe() | ||
except NotImplementedError: | ||
orbeckst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for i, ts in enumerate(ProgressBar(self._trajectory[self.start:self.stop:self.step], verbose=True, | ||
postfix=f'running system {self._key}')): | ||
self._frame_index = i | ||
self._ts = ts | ||
self.frames[i] = ts.frame | ||
self.times[i] = ts.time | ||
self._single_frame() | ||
self._conclude_universe() | ||
self._frame_index = i | ||
self._ts = ts | ||
self.frames[i] = ts.frame | ||
self.times[i] = ts.time | ||
self._single_frame() | ||
logger.info("Moving to next universe") | ||
logger.info("Finishing up") | ||
self._conclude_ensemble() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,3 +93,8 @@ def test_ValueError_different_ensemble(self): | |
match='Dihedral selections from different Ensembles, '): | ||
DihedralAnalysis([dh1, dh2]) | ||
|
||
def test_single_universe(self): | ||
dh = self.Ens.select_atoms('name C4', 'name C17', 'name S2', 'name N3') | ||
with pytest.raises(NotImplementedError): | ||
DihedralAnalysis([dh])._single_universe() | ||
|
||
Comment on lines
+96
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is not strictly necessary but we can leave it in, as a check that the DihedralAnalysis does not do something weird to |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,6 +149,38 @@ def _conclude_universe(self): | |
TestRun = TestAnalysis(Sim).run(start=0, step=1, stop=10) | ||
assert Sim.keys() == TestRun.key_list | ||
|
||
def test_ensemble_analysis_run_frame(self): | ||
class TestAnalysis(EnsembleAnalysis): | ||
def __init__(self, test_ensemble): | ||
super(TestAnalysis, self).__init__(test_ensemble) | ||
|
||
self._ens = test_ensemble | ||
|
||
def _single_universe(self): | ||
pass | ||
|
||
Sim = Ensemble(dirname=self.tmpdir.name, solvents=['water']) | ||
TestRun = TestAnalysis(Sim) | ||
|
||
with pytest.raises(NotImplementedError): | ||
TestRun._single_frame() | ||
|
||
def test_ensemble_analysis_run_universe(self): | ||
class TestAnalysis(EnsembleAnalysis): | ||
def __init__(self, test_ensemble): | ||
super(TestAnalysis, self).__init__(test_ensemble) | ||
|
||
self._ens = test_ensemble | ||
|
||
def _single_frame(self): | ||
pass | ||
|
||
Sim = Ensemble(dirname=self.tmpdir.name, solvents=['water']) | ||
TestRun = TestAnalysis(Sim) | ||
|
||
with pytest.raises(NotImplementedError): | ||
TestRun._single_universe() | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @orbeckst Can you provide more detail for the linked comment? This test might not have shown up with the recent commit, but if it did, can you elaborate on how it is not correctly testing the base class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codecov indicates that these lines were not tested. Codecov is not always right but it's something to look into carefully. Come up with a way to convince yourself (and me) that your tests really test the code that you think they do. Perhaps temporarily change the raise NotImplementError to ValueError and then your test should fail. Report back on what you did. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed exception/error types and nothing changed. If the test and class definitions were nested correctly, pytest would have also picked up that I split the single test into two separate, one for There is probably a way to consolidate the two tests back into one, but it is very clear what is being done this way. It still seems there is a simpler method. I will review again in the morning and make changes requested for documentation so this can be merged if everything is correct otherwise! |
||
def test_value_error(self): | ||
ens = Ensemble(dirname=self.tmpdir.name, solvents=['water']) | ||
copy_ens = Ensemble() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test that checks that this NotImplementedError is raised. If necessary by calling the method explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/Becksteinlab/MDPOW/blob/ensemble_run_update/mdpow/tests/test_dihedral.py
I am going to add explicit tests for raising NotImplementedError for _single_frame() and _single_universe() similar to pattern in your comment above, placed in test_ensemble.py, because test_run.py is for the partition coefficient calculation and test_ensemble.py deals with EnsembleAnalysis class where this run method that calls single_frame/uni is defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look into https://github.com/Becksteinlab/MDPOW/pull/216/files and you see the annotation by codecov that says that the line was not covered by tests.
Also check the codecov report in the checks and ultimately https://app.codecov.io/gh/Becksteinlab/MDPOW/pull/216