Skip to content
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
e71456d
developing back end run methods
cadeduckworth Sep 15, 2022
6c67ec1
condensed backend run methods
cadeduckworth Sep 15, 2022
7bcdf49
Addressed change requests by Oliver
cadeduckworth Oct 22, 2022
a87cb11
fixed indentation error for _single_frame() block
cadeduckworth Oct 22, 2022
7edde0d
reverting previous tab issue, necessary for normal function
cadeduckworth Oct 22, 2022
48016c5
fixed errors previously reverted for testing
cadeduckworth Oct 23, 2022
59c819c
Merge branch 'develop' into ensemble_run_update
cadeduckworth Oct 23, 2022
fb58f27
doc string format/syntax issue
cadeduckworth Oct 25, 2022
1f7f2d1
checkout from alt repository
cadeduckworth Oct 25, 2022
179603f
changed base class function definitions from pass to raise NotImpleme…
cadeduckworth Oct 28, 2022
46d3cf2
Merge branch 'ensemble_run_update' of github.com:Becksteinlab/MDPOW i…
cadeduckworth Oct 28, 2022
f47c806
adding preliminary test for try-except pattern addition to backend ru…
cadeduckworth Nov 2, 2022
5e02322
removed # pragma: no cover from _single_frame() and _single_universe()
cadeduckworth Nov 4, 2022
2fc9135
changed dihedral atom group selection strings in test_dihedral.py to …
cadeduckworth Nov 4, 2022
37cd1d7
changed dihedral atom group selection order in test_dataframe to matc…
cadeduckworth Nov 4, 2022
0dc7e32
several changes and updates to test_dihedral.py, resulting in passing…
cadeduckworth Nov 12, 2022
a0de2d9
relocating test to external branch, 214_dihedral issue
cadeduckworth Nov 12, 2022
273c1af
moved raise NotImplementedError test for _single_universe() for Dihed…
cadeduckworth Nov 12, 2022
8212d11
Merge remote-tracking branch 'origin/214_dihedralanalysis-test-issues…
cadeduckworth Nov 12, 2022
653210e
added explicit backend run method tests for EnsembleAnalysis
cadeduckworth Nov 17, 2022
7eff11f
test_dihedral.py reflects same changes as PR#218
cadeduckworth Nov 17, 2022
67261cb
Merge branch 'develop' into ensemble_run_update
orbeckst Nov 17, 2022
2f63c3a
Merge branch 'develop' into ensemble_run_update
orbeckst Nov 17, 2022
ce02b3b
Merge branch 'develop' into ensemble_run_update
orbeckst Nov 18, 2022
cf4bd40
updated source and html docs and CHANGELOG to reflect changes made fo…
cadeduckworth Dec 8, 2022
f45fc6b
merge with origin changes
cadeduckworth Dec 8, 2022
1a90ed8
split EnsembleAnalysis run method test, one test for _single_frame, o…
cadeduckworth Dec 8, 2022
7054bb5
minor change to run method tests for proper use of example EnsembleAn…
cadeduckworth Dec 8, 2022
19f7c84
added and corrected use of sphinx markup for documentation, edited ve…
cadeduckworth Dec 8, 2022
40381aa
changes to docs for sphinx markup
cadeduckworth Dec 8, 2022
8870e39
whitespace and docstring formatting
cadeduckworth Dec 11, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mdpow/analysis/dihedral.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def check_dihedral_inputs(selections):
for group in selections:
for k in group.keys():
if len(group[k]) != 4:
msg = ''''Dihedral calculations require AtomGroups with
msg = '''Dihedral calculations require AtomGroups with
only 4 atoms, %s selected''' % len(group)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

reformat this message so that it does not contain newline/space

Suggested change
msg = '''Dihedral calculations require AtomGroups with
only 4 atoms, %s selected''' % len(group)
msg = ("Dihedral calculations require AtomGroups with "
f"only 4 atoms, {len(group)} selected")

And we can use f-strings.

logger.error(msg)
raise SelectionError(msg)
Expand Down
31 changes: 19 additions & 12 deletions mdpow/analysis/ensemble.py
Original file line number Diff line number Diff line change
Expand Up @@ -469,14 +469,14 @@ def _single_universe(self):
Run on each universe in the ensemble during when
self.run in called.
"""
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.
"""
pass # pragma: no cover
raise NotImplementedError
Copy link
Copy Markdown
Member

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

def test_single_frame_raises_NotImplementedError():
    ...
    ea = EnsembleAnalysis(...)
    with pytest.raises(NotImplementedError):
          ea._single_frame()

Copy link
Copy Markdown
Contributor Author

@cadeduckworth cadeduckworth Nov 17, 2022

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

  • file does not exist in pull request because it was deleted and moved to another pull request, so I just linked the source from the branch
  • function is at end of module
  • this is the explicit call of _single_universe() to test NotImplementedError is raised when not defined, but specifically moved for use in DihedralAnalysis as instructed

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

  • will be in this pull request

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -505,27 +505,34 @@ 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 _single_universe on each system or _single_frame
on each frame in the system.

First iterates through keys of ensemble, then runs _setup_system
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add markup

:meth:`_setup_system`

which defines the system and trajectory. Then iterates over
trajectory frames.

NotImplementedError will detect whether _single_universe or _single_frame
should be implemented, based on which is defined in the EnsembleAnalysisClass.
Only one of the two aforementioned functions should be defined for the respective
analysis class. For verbose functionality, the analysis will currently show two
iteration bars, where only one of which will actually be iterated, while the other
will load to completion instantaneously, showing the system that is being worked on.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These implementation details are developer documentation and do not need to be in run() — users read these docs and will be confused. Put it into the part of the docs that talks about EnsembleAnalysis and how to write your own. Add a section to https://mdpow.readthedocs.io/en/latest/analysis/ensemble_analysis.html

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do I need to create a Read the Docs account and link my GitHub account and repository to make these changes as indicated in the Sphinx documentation, or do I make the changes to the text files (i.e. ensemble_analysis.txt) and push the changes straight to GitHub?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You don't need an account. The CI is already using RTD.

You only need to edit the docs here, namely ensemble_analysis.txt.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The CI puts an updated view of the docs for the PR at https://mdpow--216.org.readthedocs.build/en/216/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I think everything merged and pushed correctly, and all tests passed. I do need to modify the docs slightly after following the link above, but will wait until your review to make all necessary documentation changes in one commit.

"""
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:
Comment thread
orbeckst marked this conversation as 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()
Expand Down
41 changes: 23 additions & 18 deletions mdpow/tests/test_dihedral.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@

class TestDihedral(object):
DG48910_mean = -172.9849512527183
DG491011_mean = -3.7300197060478695
DG48910_var = 1490.6576365537262
DG491011_var = 128.3805265432388
DG491011_mean = 177.74725233051953
DG48910_var = 0.20311120667628546
DG491011_var = 0.006976126708773456
Comment thread
orbeckst marked this conversation as resolved.
Outdated

def setup(self):
self.tmpdir = td.TempDir()
Expand All @@ -37,25 +37,25 @@ def teardown(self):
self.tmpdir.dissolve()

def test_dataframe(self):
dh1 = self.Ens.select_atoms('name C4 or name C17 or name S2 or name N3')
dh1 = self.Ens.select_atoms('name C4', 'name C17', 'name S2', 'name N3')
Comment thread
orbeckst marked this conversation as resolved.
Outdated
dh_run = DihedralAnalysis([dh1]).run(start=0, stop=4, step=1)

results = dh_run.results

assert results['selection'][0] == 'S2-N3-C4-C17'
assert results['selection'][0] == 'C4-C17-S2-N3'
Comment thread
orbeckst marked this conversation as resolved.
for s in results['solvent']:
assert s == 'water'
for i in results['interaction'][:12]:
assert i == 'Coulomb'

def test_selection_error(self):
dh1 = self.Ens.select_atoms('name C17 or name S2 or name N3')
dh1 = self.Ens.select_atoms('name C17', 'name S2', 'name N3')
Comment thread
orbeckst marked this conversation as resolved.
with pytest.raises(SelectionError):
dh_run = DihedralAnalysis([dh1]).run(start=0, stop=4, step=1)

def test_results_recursive1(self):
dh1 = self.Ens.select_atoms('name C11 or name C10 or name C9 or name C4')
dh2 = self.Ens.select_atoms('name C11 or name C10 or name C9 or name C4')
dh1 = self.Ens.select_atoms('name C11', 'name C10', 'name C9', 'name C4')
dh2 = self.Ens.select_atoms('name C11', 'name C10', 'name C9', 'name C4')
Comment thread
orbeckst marked this conversation as resolved.
Outdated

dh_run1 = DihedralAnalysis([dh1]).run(start=0, stop=4, step=1)
dh_run2 = DihedralAnalysis([dh2]).run(start=0, stop=4, step=1)
Expand All @@ -64,29 +64,34 @@ def test_results_recursive1(self):
assert dh_run1.results['dihedral'][i] == dh_run2.results['dihedral'][i]

def test_results_recursive2(self):
dh1 = self.Ens.select_atoms('name C11 or name C10 or name C9 or name C4')
dh2 = self.Ens.select_atoms('name C8 or name C4 or name C9 or name C10')
dh1 = self.Ens.select_atoms('name C11', 'name C10', 'name C9', 'name C4')
dh2 = self.Ens.select_atoms('name C8', 'name C4', 'name C9', 'name C10')
Comment thread
orbeckst marked this conversation as resolved.
Outdated

dh_run = DihedralAnalysis([dh1, dh2]).run(start=0, stop=4, step=1)

dh1_result = dh_run.results.loc[dh_run.results['selection'] == 'C4-C9-C10-C11']['dihedral']
dh2_result = dh_run.results.loc[dh_run.results['selection'] == 'C4-C8-C9-C10']['dihedral']
dh1_result = dh_run.results.loc[dh_run.results['selection'] == 'C11-C10-C9-C4']['dihedral']
dh2_result = dh_run.results.loc[dh_run.results['selection'] == 'C8-C4-C9-C10']['dihedral']
Comment thread
orbeckst marked this conversation as resolved.
Outdated

dh1_mean = circmean(dh1_result, high=180, low=-180)
dh2_mean = circmean(dh2_result, high=180, low=-180)
dh1_var = circvar(dh1_result, high=180, low=-180)
dh2_var = circvar(dh2_result, high=180, low=-180)

assert_almost_equal(self.DG48910_mean, dh1_mean, 6)
assert_almost_equal(self.DG48910_var, dh1_var, 6)
assert_almost_equal(self.DG491011_mean, dh2_mean, 6)
assert_almost_equal(self.DG491011_var, dh2_var, 6)
assert_almost_equal(dh1_mean, self.DG48910_mean, 6)
assert_almost_equal(dh1_var, self.DG48910_var, 6)
assert_almost_equal(dh2_mean, self.DG491011_mean, 6)
assert_almost_equal(dh2_var, self.DG491011_var, 6)
Comment thread
orbeckst marked this conversation as resolved.
Outdated

def test_ValueError_different_ensemble(self):
other = Ensemble(dirname=self.tmpdir.name, solvents=['water'])
dh1 = self.Ens.select_atoms('name C11 or name C10 or name C9 or name C4')
dh2 = other.select_atoms('name C8 or name C4 or name C9 or name C10')
dh1 = self.Ens.select_atoms('name C11', 'name C10', 'name C9', 'name C4')
dh2 = other.select_atoms('name C8', 'name C4', 'name C9', 'name C10')
Comment thread
orbeckst marked this conversation as resolved.
Outdated
with pytest.raises(ValueError,
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 _single_universe().