-
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 26 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 |
---|---|---|
|
@@ -5,7 +5,26 @@ CHANGES for MDPOW | |
Add summary of changes for each release. Use ISO 8061 dates. Reference | ||
GitHub issues numbers and PR numbers. | ||
|
||
2022-12-07 0.8.1 | ||
cadeduckworth | ||
|
||
Changes | ||
|
||
* for ensemble.EnsembleAnalysis._single_frame() | ||
changed 'pass' to 'raise NotImplementedError' (#216) | ||
* for ensemble.EnsembleAnalysis._single_universe() | ||
changed 'pass' to 'raise NotImplementedError' (#216) | ||
* for ensemble.EnsembleAnalysis.run() changed to detect | ||
and use _single_frame OR _single_universe (#216) | ||
* _prepare_universe and _conclude_universe removed from | ||
EnsembleAnalysis.run() method, no longer needed (per comments, #199) | ||
* defined new test for updated ensemble.EnsembleAnalysis.run() method (#216): | ||
- new test, test_ensemble_analysis_run(), uses TestAnalysis class, | ||
is in test_ensemble.py | ||
- new test checks that NotImplementedError is raised when _single_frame | ||
or _single_universe are not defined, to implement correct method | ||
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. remove — CHANGES only lists what's important to users |
||
* source and html docs updated for EnsembleAnalysis | ||
to reflect changes and describe proper usage | ||
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. remove — not relevant to users (this goes in the commit message) CHANGES does not just collect commit messages, it summarizes changes at a high level with a view towards what a user of the code needs to know when they upgrade |
||
|
||
2022-??-?? 0.8.1 | ||
cadeduckworth, orbeckst, VOD555 | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,10 +15,17 @@ Universes and be analyzed as a group. | |||||
|
||||||
:class:`~mdpow.analysis.ensemble.EnsembleAnalysis` is a class inspired by the | ||||||
:class:`AnalysisBase <MDAnalysis.analysis.base.AnalysisBase>` from MDAnalysis which | ||||||
iterates over the systems in the ensemble and the frames in the systems. It sets up both iterations between | ||||||
universes and universe frames allowing for analysis to be run on both whole systems and the frames of those | ||||||
iterates over the systems in the ensemble or the frames in the systems. It sets up iterations between | ||||||
universes or universe frames allowing for analysis to be run on either whole systems or the frames of those | ||||||
systems. This allows for users to easily run analyses on MDPOW simulations. | ||||||
|
||||||
NotImplementedError will detect whether _single_universe or _single_frame | ||||||
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. Use sphinx reST markup
Suggested change
For specific Python directives and roles see https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#the-python-domain (leave out a leading 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. To actually make the markup link to the doc, use something like :meth:`~EnsembleAnalysis._single_universe` The tilde will make it appear as only |
||||||
should be implemented, based on which is defined in the EnsembleAnalysisClass. | ||||||
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. mark up :class:`EnsembleAnalysis` Note that this works (or should work) because you're currently documenting the But if it doesn't work (i.e., you go to the HTML docs on RTD and clicking on :class:`mdpow.analysis.ensemble.EnsembleAnalysis` if you want to see the full "path" or :class:`~mdpow.analysis.ensemble.EnsembleAnalysis` if you only want the class name. |
||||||
Only one of the two methods should be defined for an EnsembleAnalysisClass. | ||||||
For verbose functionality, the analysis may 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. | ||||||
|
||||||
.. autoclass:: mdpow.analysis.ensemble.EnsembleAnalysis | ||||||
:members: | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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) | ||||||||||
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. reformat this message so that it does not contain newline/space
Suggested change
And we can use f-strings. |
||||||||||
logger.error(msg) | ||||||||||
raise SelectionError(msg) | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -466,17 +466,25 @@ def _setup_frames(self, trajectory): | |||||
def _single_universe(self): | ||||||
"""Calculations on a single Universe object. | ||||||
|
||||||
Run on each universe in the ensemble during when | ||||||
self.run in called. | ||||||
Run on each universe in the ensemble during when | ||||||
self.run in called. | ||||||
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.
Suggested change
|
||||||
|
||||||
NotImplementedError will detect whether _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. mark-up |
||||||
or _single_frame should be implemented, based on which | ||||||
is defined in the EnsembleAnalysisClass. | ||||||
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. is defined in the :class:`EnsembleAnalysis` class. Only use the proper identifiers, don't make any up by adding "Class" to the end. |
||||||
""" | ||||||
pass # pragma: no cover | ||||||
raise NotImplementedError | ||||||
|
||||||
def _single_frame(self): | ||||||
"""Calculate data from a single frame of trajectory | ||||||
"""Calculate data from a single frame of trajectory. | ||||||
|
||||||
Called on each frame for universes in the Ensemble. | ||||||
Called on each frame for universes in the Ensemble. | ||||||
|
||||||
NotImplementedError will detect whether _single_universe | ||||||
or _single_frame should be implemented, based on which | ||||||
is defined in the EnsembleAnalysisClass. | ||||||
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. mark up etc |
||||||
""" | ||||||
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. 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() 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. 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 commentThe 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 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 +513,27 @@ 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 | ||||||
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. add markup :meth:`_setup_system` |
||||||
which defines the system and trajectory. Then iterates over | ||||||
trajectory frames. | ||||||
which defines the system and trajectory. Then iterates over each | ||||||
system universe or trajectory frames of each universe as defined. | ||||||
""" | ||||||
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,21 @@ 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(self): | ||
class TestAnalysis(EnsembleAnalysis): | ||
def __init__(self, test_ensemble): | ||
super(TestAnalysis, self).__init__(test_ensemble) | ||
|
||
def test_single_frame(self): | ||
with pytest.raises(NotImplementedError) as excinfo: | ||
TestAnalysis._single_frame(self) | ||
assert excinfo.type == 'NotImplementedError' | ||
|
||
def test_single_universe(self): | ||
with pytest.raises(NotImplementedError) as excinfo: | ||
TestAnalysis._single_universe(self) | ||
assert excinfo.type == '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. @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.
change to 0.9.0 — we are making API changes so under semantic versioning this cannot be a patch release.
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.
turn the date into 2022-??-?? — we don't know a release date yet
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.
In fact: add everything to the 0.8.1 entry below and change that 0.8.1 to 0.9.0