Skip to content

developing back end run methods #216

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.

Sign up for GitHub

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

Merged
orbeckst merged 31 commits into from
Dec 12, 2022
Merged

developing back end run methods #216

Show file tree
Hide file tree
Changes from 2 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
19 changes: 19 additions & 0 deletions CHANGES
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

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.

Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

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

Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

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

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
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

The 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
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

The 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
Expand Down
11 changes: 9 additions & 2 deletions doc/sphinx/source/analysis/ensemble_analysis.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

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

Use sphinx reST markup

Suggested change
NotImplementedError will detect whether _single_universe or _single_frame
:exc:`NotImplementedError` will detect whether :meth:`_single_universe` or :meth:`_single_frame`

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 :py: because that's the default for us). See also https://www.sphinx-doc.org/en/master/usage/restructuredtext/index.html for more on reST.

Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

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

should be implemented, based on which is defined in the EnsembleAnalysisClass.
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

The 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 mdpow.analysis.ensemble module.

But if it doesn't work (i.e., you go to the HTML docs on RTD and clicking on EnsembleAnalysis does NOT lead you to the full docs) then write it as

: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:

Expand Down
27 changes: 14 additions & 13 deletions mdpow/analysis/ensemble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,15 +466,23 @@ 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.
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

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

Suggested change
self.run in called.
:meth:`run` is called.


NotImplementedError will detect whether _single_universe
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

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

mark-up NotImplementedError and _single_universe and _single_frame

or _single_frame should be implemented, based on which
is defined in the EnsembleAnalysisClass.
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

The 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.

"""
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.
Copy link
Copy Markdown
Member

@orbeckst orbeckst Dec 8, 2022

Choose a reason for hiding this comment

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

mark up etc

"""
raise NotImplementedError
Copy link
Copy Markdown
Member

@orbeckst orbeckst Nov 14, 2022

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 •
edited
Loading

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

@orbeckst orbeckst Dec 8, 2022

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

@orbeckst orbeckst Dec 8, 2022

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.


Expand Down Expand Up @@ -509,15 +517,8 @@ def run(self, start=None, stop=None, step=None):
on each frame in the system.

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

@orbeckst orbeckst Dec 8, 2022

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.
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()
Expand Down